feat: add product-details block #488
Conversation
WalkthroughAdds two new block packages (product-details, recommended-products) with API, SDK, frontend (client/server/renderer), types, Nest modules/controllers/services, mappers, mock data, UI components, Storybook stories, tsconfigs and tooling, and integrates them into apps, framework models, and CMS integrations. Changes
Sequence Diagram(s)sequenceDiagram
actor Browser
participant FE as Frontend Renderer
participant SSR as Next Server (Server Component)
participant SDK as Block SDK
participant API as Block API (NestJS)
participant Service as Block Service
participant CMS as CMS Service
participant Products as Products Service
Browser->>FE: request product page (id, locale)
FE->>SSR: render server component (id, locale)
SSR->>SDK: sdk.blocks.getProductDetails({id}, query, headers)
SDK->>API: HTTP GET /blocks/product-details/{id}
API->>Service: getProductDetails(id, query, headers)
par Parallel fetch
Service->>CMS: getProductDetailsBlock({id, locale})
Service->>Products: getProduct({id})
end
CMS-->>Service: cms block
Products-->>Service: product data
Service->>Service: mapProductDetails(...)
Service-->>API: ProductDetailsBlock / 404
API-->>SDK: JSON response
SDK-->>SSR: data
SSR->>FE: render client component with data
FE->>Browser: final UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
…ment and null handling
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
3-37: Identical placeholder implementation.This mapper is identical to the Contentful implementation. See the review comment on
packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.tsfor details.
🧹 Nitpick comments (17)
packages/blocks/product-details/vitest.config.mjs (1)
1-1: LGTM! Standard config re-export pattern.The file correctly imports and re-exports the shared Vitest configuration, which is a common and appropriate pattern for maintaining consistent testing setup across blocks in a monorepo.
Optionally, consider splitting the import and export statements across multiple lines for better readability:
-import { config } from '@o2s/vitest-config/block'; export default config; +import { config } from '@o2s/vitest-config/block'; + +export default config;packages/integrations/mocked/src/modules/products/products.mapper.ts (1)
322-393: Consider aligning image URLs with the established pattern and removing redundancy.The new mock product uses
unsplash.comURLs for images, while all other mock products consistently useraw.githubusercontent.comURLs. External URLs may introduce stability concerns (URL changes, attribution requirements, network availability) and break the established pattern for mock data.Additionally, the
imagefield (lines 330-335) and the first item in theimagesarray (lines 337-342) share the same URL. This redundancy can be eliminated.🔎 Suggested improvements
Option 1: Use consistent githubusercontent.com URLs
Replace unsplash URLs with githubusercontent URLs matching the pattern used by other products.Option 2: Remove image field redundancy
If keeping the current URLs, consider whether bothimageandimages[0]need the same URL. Based on the api-harmonization layer (external_code_snippet_1),imageserves as a fallback whenimagesis undefined, so having both defined with identical content may be unnecessary.Option 3: Verify icon names
Ensure the icon string references inkeySpecs('Calendar', 'CheckCircle', 'Fuel', 'Settings') match the actual icon library being used in the frontend.packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
10-15: Consider simplifying the images type.The conditional type logic for the
imagesfield is complex and may be difficult to maintain. Since the API harmonization layer likely uses a mapper to ensure images are always populated, consider documenting this requirement or simplifying the type.Alternative approach
If the mapper guarantees images are always present, you could simplify to:
export type Product = Products.Model.Product & { badges?: Badge[]; - images: - | NonNullable<Products.Model.Product['images']> - | (Products.Model.Product['image'] extends undefined ? never : [Products.Model.Product['image']]); + images: Media.Media[]; };And ensure the mapper always populates this field from either
imagesorimage.packages/blocks/product-details/src/api-harmonization/product-details.service.spec.ts (1)
1-62: Consider adding behavior tests for service methods.The test setup correctly instantiates the service with mocked dependencies following NestJS testing patterns. However, the tests only verify that services are defined without testing any actual behavior of
ProductDetailsService.Consider adding tests that verify:
- How
ProductDetailsServiceusescmsService.getProductDetailsBlock- How it integrates with
productsServicemethods- Error handling scenarios
- Data transformation logic
This would provide better coverage and confidence in the service implementation.
packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
3-18: Placeholder implementation - consider following the mocked CMS pattern for locale support.The mapper currently returns static English labels regardless of locale. When implementing proper Contentful mapping, reference the mocked CMS implementation (
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts) which demonstrates locale-aware label switching.Note: This comment also applies to the identical Strapi implementation in
packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts.packages/blocks/product-details/src/sdk/index.ts (1)
16-19: Consider fixing logger type definitions instead of suppressing errors.The
@ts-expect-errordirectives indicate missing type definitions forlevelandformatin the logger configuration. If this is a recurring pattern, consider updating the logger type definitions in@o2s/framework/sdkto include these properties.packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
3-37: Placeholder implementation - identical to Strapi mapper.This mapper is duplicated in
packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.tswith identical structure. When implementing proper Contentful mapping, consider whether shared stub data could be extracted to reduce duplication.Note: Unlike product-details, there's no locale-aware mocked implementation yet to use as a reference for this block type.
packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
3-17: Acknowledge placeholder implementation.The mapper currently returns a hardcoded structure with a TODO for proper Strapi mapping. While this is acceptable for initial integration, ensure the TODO is tracked so that proper CMS field mapping is implemented before production use.
Do you want me to open an issue to track the Strapi mapping implementation?
packages/integrations/strapi-cms/src/modules/cms/cms.service.ts (1)
450-456: Consider adding caching for consistency.Unlike most other block methods in this service (which use
getCachedBlock),getProductListBlockandgetProductDetailsBlockreturn observables directly without caching. This divergence from the established pattern may lead to unnecessary repeated mapper calls.🔎 Proposed refactor to add caching
getProductListBlock(options: CMS.Request.GetCmsEntryParams) { - return of(mapProductListBlock(options.locale)); + const key = `product-list-component-${options.id}-${options.locale}`; + return this.getCachedBlock(key, () => of(mapProductListBlock(options.locale))); } getProductDetailsBlock(options: CMS.Request.GetCmsEntryParams) { - return of(mapProductDetailsBlock(options.locale)); + const key = `product-details-component-${options.id}-${options.locale}`; + return this.getCachedBlock(key, () => of(mapProductDetailsBlock(options.locale))); }packages/blocks/product-details/src/frontend/ProductDetails.server.tsx (1)
32-35: Consider improving error handling by providing user feedback.Returning
nullon error silently hides the component without any user-visible indication. Consider rendering an error state component or using an error boundary pattern to provide feedback.🔎 Suggested improvement
} catch (error) { console.error('Error fetching ProductDetails block', error); - return null; + return ( + <div className="text-destructive p-4"> + Unable to load product details + </div> + ); }packages/blocks/product-details/src/api-harmonization/product-details.service.ts (1)
34-40: Consider fetching more items to ensure 4 popular offers after filtering.Fetching exactly 4 items at
limit: 4may result in fewer than 4 popular offers after filtering out the current product. Consider fetching a few extra (e.g.,limit: 5) to account for the exclusion.🔎 Suggested improvement
const popularOffers = this.productsService.getProductList({ - limit: 4, + limit: 5, offset: 0, locale, });packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (2)
245-245: Address TODO: Recommended products rendering is incomplete.The
popularOfferssection renders a placeholder<h3>TODO: Recommended products</h3>instead of actual product cards. This should be implemented before merging or tracked as a follow-up issue.Would you like me to help implement the recommended products grid, or should I open a new issue to track this task?
219-228: Consider memoizing the action button click handler.If
actionButton.onClickis a new function reference on each render, it could cause unnecessary re-renders. Consider wrapping it withuseCallbackif performance becomes a concern.Also applies to: 270-279
packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (1)
38-43: Consider making the action button's icon and variant configurable.The
icon: 'MessageCircle'andvariant: 'default'are hardcoded. If different product types require different icons or styling, consider adding these fields to the CMS labels.packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx (1)
106-161: Consider extracting common mock data to constants.The
popularOffersarray and other data structures are duplicated across multiple stories (e.g., lines 106-161, 285-323). While this duplication makes each story self-contained, it increases maintenance burden when product data needs updating.Consider extracting frequently reused data to constants:
const MOCK_POPULAR_OFFERS = [ { id: 'PRD-005', name: 'Cordless Angle Grinder', /* ... */ }, { id: 'PRD-006', name: 'Laser Measurement', /* ... */ }, { id: 'PRD-007', name: 'Cordless Drill Driver', /* ... */ }, ]; const DEFAULT_ROUTING = { locales: ['en', 'pl'], defaultLocale: 'en', pathnames: { /* ... */ }, };Then reference them in stories:
popularOffers: MOCK_POPULAR_OFFERS.packages/ui/src/components/ProductGallery/ProductGallery.tsx (2)
62-80: LGTM: Proper keyboard and scroll handling.The ESC key handling and body scroll locking follow the standard pattern for modal components. The cleanup function correctly restores the original state.
For more robust scroll locking (especially with nested modals or multiple components), consider using a dedicated library like
react-remove-scrollorbody-scroll-lock-upgradethat handles edge cases like iOS Safari and nested locks.
87-128: LGTM: Well-implemented main gallery.The Swiper configuration is comprehensive with proper null checks (line 105 checking both existence and
destroyedstate). The conditional navigation opacity (lines 94-96) provides good UX. Image optimization withpriorityflag and responsivesizesis properly implemented.Line 111 uses array index as key. While acceptable for static image galleries, consider using stable identifiers (e.g.,
image.urlor a unique ID) if images might be reordered dynamically:-<SwiperSlide key={index}> +<SwiperSlide key={image.url}>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
-
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (51)
.gitignoreapps/api-harmonization/package.jsonapps/api-harmonization/src/app.module.tsapps/api-harmonization/src/modules/page/page.model.tsapps/frontend/package.jsonapps/frontend/src/blocks/renderBlocks.tsxpackages/blocks/product-details/.gitignorepackages/blocks/product-details/.prettierrc.mjspackages/blocks/product-details/eslint.config.mjspackages/blocks/product-details/lint-staged.config.mjspackages/blocks/product-details/package.jsonpackages/blocks/product-details/src/api-harmonization/index.tspackages/blocks/product-details/src/api-harmonization/product-details.client.tspackages/blocks/product-details/src/api-harmonization/product-details.controller.tspackages/blocks/product-details/src/api-harmonization/product-details.mapper.tspackages/blocks/product-details/src/api-harmonization/product-details.model.tspackages/blocks/product-details/src/api-harmonization/product-details.module.tspackages/blocks/product-details/src/api-harmonization/product-details.request.tspackages/blocks/product-details/src/api-harmonization/product-details.service.spec.tspackages/blocks/product-details/src/api-harmonization/product-details.service.tspackages/blocks/product-details/src/api-harmonization/product-details.url.tspackages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsxpackages/blocks/product-details/src/frontend/ProductDetails.client.tsxpackages/blocks/product-details/src/frontend/ProductDetails.renderer.tsxpackages/blocks/product-details/src/frontend/ProductDetails.server.tsxpackages/blocks/product-details/src/frontend/ProductDetails.types.tspackages/blocks/product-details/src/frontend/index.tspackages/blocks/product-details/src/sdk/index.tspackages/blocks/product-details/src/sdk/product-details.tspackages/blocks/product-details/tsconfig.api.jsonpackages/blocks/product-details/tsconfig.frontend.jsonpackages/blocks/product-details/tsconfig.jsonpackages/blocks/product-details/tsconfig.sdk.jsonpackages/blocks/product-details/vitest.config.mjspackages/framework/src/modules/cms/cms.model.tspackages/framework/src/modules/cms/cms.service.tspackages/framework/src/modules/cms/models/blocks/product-details.model.tspackages/framework/src/modules/products/products.model.tspackages/integrations/contentful-cms/src/modules/cms/cms.service.tspackages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.tspackages/integrations/mocked/src/modules/cms/cms.service.tspackages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/mocked/src/modules/products/products.mapper.tspackages/integrations/strapi-cms/src/modules/cms/cms.service.tspackages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.tspackages/ui/src/components/ProductGallery/ProductGallery.stories.tsxpackages/ui/src/components/ProductGallery/ProductGallery.tsxpackages/ui/src/components/ProductGallery/ProductGallery.types.tspackages/ui/src/components/ProductGallery/index.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
📚 Learning: 2025-11-26T11:57:00.632Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
Applied to files:
packages/blocks/product-details/src/api-harmonization/product-details.request.tspackages/framework/src/modules/cms/cms.service.tspackages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.tspackages/blocks/product-details/tsconfig.sdk.jsonpackages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/blocks/product-details/package.jsonpackages/integrations/contentful-cms/src/modules/cms/cms.service.tsapps/frontend/src/blocks/renderBlocks.tsxpackages/blocks/product-details/src/api-harmonization/product-details.mapper.tsapps/api-harmonization/src/modules/page/page.model.tspackages/framework/src/modules/cms/models/blocks/product-details.model.tspackages/blocks/product-details/src/frontend/ProductDetails.types.tspackages/framework/src/modules/cms/cms.model.tspackages/blocks/product-details/tsconfig.api.jsonpackages/integrations/mocked/src/modules/cms/cms.service.tspackages/blocks/product-details/src/api-harmonization/product-details.model.tspackages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.tspackages/framework/src/modules/products/products.model.tspackages/blocks/product-details/tsconfig.jsonpackages/integrations/strapi-cms/src/modules/cms/cms.service.ts
📚 Learning: 2025-11-14T14:34:25.489Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 357
File: packages/blocks/ticket-summary/eslint.config.mjs:1-18
Timestamp: 2025-11-14T14:34:25.489Z
Learning: ESLint v9+ provides `defineConfig` as an export from 'eslint/config' module, which supports an `extends` array property in flat config objects for composing configurations.
Applied to files:
packages/blocks/product-details/lint-staged.config.mjspackages/blocks/product-details/.prettierrc.mjspackages/blocks/product-details/eslint.config.mjs
🧬 Code graph analysis (20)
packages/framework/src/modules/cms/cms.service.ts (3)
packages/framework/src/modules/cms/cms.request.ts (1)
GetCmsEntryParams(5-8)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
ProductDetailsBlock(45-51)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
ProductDetailsBlock(13-16)
packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (4)
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(42-53)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(3-18)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
ProductDetailsBlock(45-51)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
ProductDetailsBlock(13-16)
packages/ui/src/components/ProductGallery/ProductGallery.types.ts (1)
packages/ui/src/components/ProductGallery/index.ts (2)
ImageItem(2-2)ProductGalleryProps(2-2)
packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
mapProductListBlock(3-37)
packages/blocks/product-details/lint-staged.config.mjs (1)
packages/blocks/product-details/.prettierrc.mjs (1)
config(8-23)
packages/integrations/contentful-cms/src/modules/cms/cms.service.ts (5)
packages/framework/src/modules/cms/cms.request.ts (1)
GetCmsEntryParams(5-8)packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
mapProductListBlock(3-37)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
mapProductListBlock(3-37)packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(3-18)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(3-18)
packages/ui/src/components/ProductGallery/ProductGallery.tsx (3)
packages/ui/src/components/ProductGallery/index.ts (2)
ProductGallery(1-1)ProductGalleryProps(2-2)packages/ui/src/components/ProductGallery/ProductGallery.types.ts (1)
ProductGalleryProps(8-16)packages/ui/src/lib/utils.ts (1)
cn(5-7)
packages/blocks/product-details/src/sdk/index.ts (2)
packages/framework/src/sdk.ts (1)
extendSdk(140-148)packages/blocks/product-details/src/sdk/product-details.ts (1)
productDetails(8-31)
apps/frontend/src/blocks/renderBlocks.tsx (2)
packages/blocks/product-details/src/frontend/ProductDetails.server.tsx (1)
ProductDetails(12-36)packages/blocks/product-details/src/frontend/index.ts (1)
ProductDetails(2-2)
packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (2)
packages/framework/src/modules/cms/models/blocks/product-details.model.ts (2)
ProductDetailsBlock(13-16)Labels(3-11)apps/api-harmonization/src/modules/page/page.model.ts (1)
Labels(43-43)
packages/framework/src/modules/cms/models/blocks/product-details.model.ts (2)
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (2)
Labels(35-43)ProductDetailsBlock(45-51)apps/api-harmonization/src/modules/page/page.model.ts (1)
Labels(43-43)
packages/blocks/product-details/src/frontend/ProductDetails.server.tsx (4)
packages/blocks/product-details/src/frontend/index.ts (1)
ProductDetails(2-2)packages/blocks/product-details/src/frontend/ProductDetails.types.ts (1)
ProductDetailsProps(5-12)packages/blocks/product-details/src/sdk/index.ts (1)
sdk(24-28)apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/product-details/src/frontend/ProductDetails.types.ts (2)
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
ProductDetailsBlock(45-51)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
ProductDetailsBlock(13-16)
packages/framework/src/modules/cms/cms.model.ts (2)
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
ProductDetailsBlock(45-51)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
ProductDetailsBlock(13-16)
packages/integrations/mocked/src/modules/products/products.mapper.ts (2)
packages/framework/src/modules/products/products.model.ts (2)
Products(43-43)Product(19-41)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
Product(10-15)
packages/integrations/mocked/src/modules/cms/cms.service.ts (5)
packages/framework/src/modules/cms/cms.request.ts (1)
GetCmsEntryParams(5-8)packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(3-18)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(42-53)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(3-18)packages/integrations/mocked/src/utils/delay.ts (1)
responseDelay(4-7)
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (3)
packages/framework/src/modules/products/products.model.ts (2)
Product(19-41)Products(43-43)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (2)
Labels(3-11)ProductDetailsBlock(13-16)apps/api-harmonization/src/modules/page/page.model.ts (1)
Labels(43-43)
packages/framework/src/modules/products/products.model.ts (2)
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
Product(10-15)packages/framework/src/utils/models/media.ts (1)
Media(1-7)
packages/integrations/strapi-cms/src/modules/cms/cms.service.ts (3)
packages/framework/src/modules/cms/cms.request.ts (1)
GetCmsEntryParams(5-8)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
mapProductListBlock(3-37)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(3-18)
packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx (2)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (1)
ProductDetailsPure(19-289)packages/blocks/product-details/src/frontend/index.ts (1)
ProductDetailsPure(1-1)
🔇 Additional comments (61)
.gitignore (1)
36-36: LGTM!The addition of "agent-os" is appropriately placed alongside other IDE/editor exclusions and follows the existing .gitignore conventions.
packages/blocks/product-details/.gitignore (1)
1-57: LGTM!This is a well-structured and comprehensive .gitignore file with all standard patterns for a TypeScript/Node.js package. The selective VSCode allowlist (lines 33-36) is a good practice to preserve shared team configs. No concerns.
packages/blocks/product-details/eslint.config.mjs (1)
1-18: LGTM! Clean ESLint configuration following established patterns.The configuration correctly applies layer-specific linting rules: API config for the harmonization layer and frontend config for both the UI and SDK layers. The structure aligns with the project's ESLint v9+ flat config conventions.
Based on learnings, the
defineConfigwithextendsarrays is the correct approach for this repository.packages/integrations/mocked/src/modules/products/products.mapper.ts (1)
407-407: LGTM!The array extension correctly includes the new mock product and will work seamlessly with the existing mapping functions.
packages/blocks/product-details/.prettierrc.mjs (1)
1-25: LGTM! Well-structured Prettier configuration for multi-layer architecture.The configuration correctly applies layer-specific formatting rules using overrides. This aligns well with the product-details block's architecture spanning API harmonization, frontend, and SDK layers.
packages/blocks/product-details/src/api-harmonization/product-details.url.ts (1)
1-1: LGTM!The URL constant follows the established pattern for block endpoints and will be correctly consumed by the client, SDK, and controller layers.
packages/blocks/product-details/lint-staged.config.mjs (1)
1-3: LGTM!The configuration correctly imports and re-exports the shared base lint-staged config, maintaining consistency with other blocks in the repository.
packages/blocks/product-details/tsconfig.api.json (1)
1-14: LGTM!The TypeScript configuration correctly extends the base API config and enables the necessary decorator support for NestJS-based API harmonization modules.
packages/blocks/product-details/tsconfig.sdk.json (1)
1-20: LGTM!The SDK configuration correctly includes both the SDK source files and the specific API harmonization dependencies it requires (client, model, request, url).
packages/blocks/product-details/tsconfig.frontend.json (1)
1-23: LGTM!The frontend configuration properly extends the base frontend config and includes the necessary API harmonization and SDK dependencies for type-safe frontend development.
packages/blocks/product-details/tsconfig.json (1)
1-18: LGTM!The main TypeScript configuration correctly orchestrates the three specialized build configurations (frontend, API, SDK) using project references for efficient multi-project builds.
packages/framework/src/modules/products/products.model.ts (2)
8-17: LGTM!The new type definitions for key specifications and detailed specifications are clear and well-structured, providing the necessary flexibility for product detail rendering.
25-40: LGTM!The Product model extensions are well-designed with all new fields being optional, ensuring backward compatibility while supporting the enhanced product details block functionality.
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (4)
5-8: LGTM!The Badge type provides a clean interface for product badges with appropriate variant options.
17-25: LGTM!The ProductSummary type provides an appropriate simplified view of product data for card/list displays.
35-43: LGTM!The Labels type provides comprehensive localization support for the product details block UI.
45-51: LGTM!The ProductDetailsBlock correctly includes the
__typenamediscriminator at the API harmonization layer, consistent with the established architectural pattern.Based on learnings, the
__typenamefield is appropriately placed in the API harmonization layer for discriminated unions.apps/api-harmonization/package.json (1)
49-49: LGTM!The dependency addition follows the workspace protocol pattern used by all other block packages in this file.
packages/framework/src/modules/cms/cms.model.ts (1)
47-47: LGTM!The export alias follows the established pattern for CMS block models. The framework layer correctly maintains normalized data models without
__typenamediscriminators, which are added at the API harmonization layer as confirmed by the relevant code snippets.Based on learnings, the framework layer correctly excludes
__typenamewhich is added inpackages/blocks/product-details/src/api-harmonization/product-details.model.ts.apps/frontend/src/blocks/renderBlocks.tsx (2)
20-20: LGTM!The import follows the established pattern for block imports in this file.
146-147: LGTM!The rendering case follows the established pattern used by other blocks in this switch statement. The ProductDetails.Renderer component is properly integrated with error handling in place at the server component level.
packages/blocks/product-details/src/api-harmonization/product-details.module.ts (1)
1-20: LGTM!The module structure follows NestJS best practices. The provider alias pattern (lines 13-16) correctly maps the
Products.Servicetoken to the framework implementation, enabling dependency injection flexibility while maintaining a clean module boundary.apps/api-harmonization/src/modules/page/page.model.ts (2)
19-19: LGTM!The import statement follows the established pattern for block API harmonization imports.
82-82: LGTM!The addition to the
Blocksunion type follows the established pattern. TheProductDetailsBlock['__typename']correctly extracts the type discriminator for the union, consistent with other block types in this discriminated union.apps/api-harmonization/src/app.module.ts (1)
43-43: LGTM!The import statement follows the established pattern for block imports.
packages/blocks/product-details/src/sdk/index.ts (1)
9-11: Verify API_URL environment variable handling.The API_URL resolution uses a non-null assertion (line 14:
apiUrl: API_URL!), which will cause runtime errors if all three environment variables are undefined. Ensure that at leastNEXT_PUBLIC_API_URLis always defined in your environment configuration.packages/blocks/product-details/src/api-harmonization/product-details.request.ts (1)
1-9: Verify the need foridin both Params and Query types.Both
GetProductDetailsBlockParamsandGetProductDetailsBlockQueryinclude anidfield. Since the params are typically used for URL path segments (as seen in the SDK implementation:url: \${URL}/${params.id}`), havingidin the query type appears redundant. Confirm whether the queryid` serves a distinct purpose or should be removed.packages/integrations/contentful-cms/src/modules/cms/cms.service.ts (1)
561-567: LGTM! Consistent with existing block patterns.The new methods follow the established pattern used by other block getters in this service (e.g.,
getServiceDetailsBlock,getResourceDetailsBlock). The implementation correctly delegates to the respective mapper functions and returns an Observable.packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
42-53: LGTM! Good reference implementation for locale handling.This mapper demonstrates the proper pattern for locale-aware block mapping with appropriate fallback to English. The implementation can serve as a reference for the Contentful and Strapi mappers when they implement proper CMS integration.
packages/blocks/product-details/src/api-harmonization/index.ts (1)
1-7: LGTM! Standard barrel export pattern.The index file provides a clean public API surface for the product-details block's API harmonization layer. All necessary exports are present and follow established conventions.
packages/integrations/mocked/src/modules/cms/cms.service.ts (1)
29-29: LGTM! Consistent implementation.The import and method implementation follow the established pattern for other block types in this service. The method correctly delegates to the mapper and applies
responseDelay()for mock behavior.Also applies to: 211-213
packages/blocks/product-details/src/frontend/index.ts (1)
1-3: LGTM! Clean public API surface.The barrel export pattern with semantic aliases (Client/Server/Renderer) provides a clear and stable public API for consumers. This approach maintains good separation of concerns between client-side, server-side, and rendering logic.
packages/blocks/product-details/src/api-harmonization/product-details.controller.ts (2)
14-25: LGTM! Well-structured NestJS controller.The controller follows NestJS best practices with proper dependency injection, decorators, and delegation to the service layer. The implementation is consistent with other block controllers in the codebase.
17-18: Empty roles array is consistent with the codebase authorization pattern.The empty roles array in the product-details endpoint is intentional. Multiple similar endpoints across the blocks use the same pattern (product-list, article, category, quick-links, faq, etc.), distinguishing "public" endpoints from restricted ones that require specific roles like ORG_USER or ORG_ADMIN. This endpoint correctly follows the established design.
packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
1-16: LGTM! Correct framework layer implementation.The model correctly follows the framework layer pattern by omitting the
__typenamediscriminator (which belongs in the API harmonization layer). The structure with requiredlabelsfield and optionaltitlefield aligns with the established pattern for block models.Based on learnings: Framework layer models should not include
__typename.packages/blocks/product-details/src/frontend/ProductDetails.renderer.tsx (1)
1-41: LGTM! Well-structured renderer component.The renderer properly implements:
- Locale resolution with fallback (
propLocale || localeFromHook)- Suspense boundary with informative loading fallback
- Clean prop forwarding to the underlying
ProductDetailscomponentThe implementation follows React best practices and provides good user experience during loading states.
packages/blocks/product-details/src/api-harmonization/product-details.client.ts (1)
1-4: LGTM! Clean barrel export pattern.The file establishes a clear public API surface by aggregating related exports into logical namespaces (Model, Request) and re-exporting the URL constant. This approach provides a clean interface for API consumers.
packages/blocks/product-details/src/sdk/product-details.ts (1)
8-31: LGTM!The SDK wrapper follows the established patterns in the codebase. The header merging order is correct (defaults → custom → auth), and the conditional authorization header is properly structured.
packages/framework/src/modules/cms/cms.service.ts (1)
131-138: LGTM!The new abstract methods follow the established pattern of existing block methods in the service. Based on learnings, the framework layer correctly returns
CMS.Model.*types without__typenamediscriminators, which are added at the API harmonization layer.packages/blocks/product-details/src/frontend/ProductDetails.server.tsx (1)
27-31: Note: Suspense here wraps the dynamic import, not data fetching.Since data is fetched with
awaitbefore rendering, theSuspensefallback only appears during the dynamic component chunk load, not during data fetching. This is correct but worth noting for clarity.packages/blocks/product-details/src/api-harmonization/product-details.service.ts (1)
41-77: LGTM on the overall service structure.The use of
forkJoinfor parallel data fetching is appropriate, and the conditional branching for popular offers is clean. TheNotFoundExceptionhandling ensures proper HTTP responses when products are missing.packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (1)
1-289: Overall structure is solid, but the component is large.The component handles responsive layouts well with proper breakpoint handling. Consider extracting sub-components (e.g.,
ProductCarousel,ProductSpecs,MobileCTA) in a future refactor to improve maintainability.packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (2)
33-45: LGTM! The__typenamediscriminator is correctly added at the API harmonization layer.Based on learnings, the framework layer maintains normalized models without
__typename, and it's appropriately added here in the API harmonization mapper for discriminated union support.
14-18: Same type cast concern as in the service.The
tag.variant as Model.Badge['variant']cast here mirrors the same pattern in the service. Ensure the source data'svariantvalues align with the expected union type.packages/blocks/product-details/src/frontend/ProductDetails.types.ts (1)
1-23: LGTM!The type definitions are well-structured. Good use of intersection types for
ProductDetailsPurePropsto combine component props with the API model, and appropriate use ofReturnType<typeof defineRouting>for type-safe routing.packages/blocks/product-details/package.json (1)
43-43: No action required — TypeScript version is current.TypeScript 5.9.3 is consistent with the current stable release (TypeScript 5.9 as of December 2025) and is intentional.
packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx (2)
1-12: LGTM! Well-structured Storybook setup.The imports, meta configuration, and type definitions follow Storybook best practices with proper type safety using the
satisfiesoperator.
182-428: Excellent story coverage!The story variants effectively demonstrate different component states:
- Discount scenarios with promotional badges
- Locale variations
- Optional features (popularOffers, actionButton)
- Minimal data edge cases
- Different button variants (link vs. onClick, variant styles)
The use of
console.login onClick handlers (lines 167, 329, 417) is appropriate for Storybook demonstrations, allowing developers to verify interactions in the browser console.packages/ui/src/components/ProductGallery/ProductGallery.stories.tsx (3)
1-17: LGTM: Clean Storybook setup.The imports, meta configuration, and type definitions follow Storybook best practices. The use of
satisfiesensures type safety while maintaining type inference.
19-49: LGTM: Well-organized sample data.The sample image generation using color-coded placeholders provides clear visual differentiation. The data sets effectively cover various gallery scenarios (single, few, many images).
51-96: LGTM: Comprehensive story coverage.The stories effectively demonstrate the component's flexibility across different scenarios (default, many images, without thumbnails, single image). The JSDoc comments clearly explain each story's purpose and expected behavior.
packages/ui/src/components/ProductGallery/index.ts (1)
1-2: LGTM: Clean barrel exports.The barrel export pattern is correctly implemented, properly separating type exports from value exports for optimal tree-shaking.
packages/ui/src/components/ProductGallery/ProductGallery.types.ts (1)
1-16: LGTM: Well-designed type definitions.The interfaces are clean and properly typed. Extending
Omit<SwiperProps, 'children'>is the right approach since the component manages its own children. ThehasPriorityprop supports image loading optimization, which is a good performance practice.packages/ui/src/components/ProductGallery/ProductGallery.tsx (8)
1-28: LGTM: Proper client component setup.The component is correctly configured as a client component with appropriate imports and default prop values. The use of
Readonly<ProductGalleryProps>enforces immutability as expected in React.
29-34: LGTM: Clean state management.The state variables appropriately track the main gallery, thumbnails, and lightbox Swiper instances. Proper null handling is set up for Swiper instance types.
36-45: LGTM: Efficient module configuration.The use of
useMemowith conditional module inclusion is the right approach. Always enabling A11y and Keyboard modules ensures good accessibility support.
51-60: LGTM: Well-optimized callback handlers.Both handlers properly use
useCallbackwith correct dependencies. The null check formainSwiperinhandleOpenLightboxprevents potential errors.
82-85: LGTM: Proper guard clause.The early return for empty or missing images prevents unnecessary rendering and potential errors.
130-163: LGTM: Clean thumbnail implementation.The thumbnail gallery correctly renders only when needed (multiple images and thumbnails enabled). The hover interaction and active state styling provide good visual feedback.
165-249: LGTM: Well-structured lightbox implementation.The lightbox overlay properly handles user interactions:
- Click-to-close on overlay (line 169)
- Click event propagation stopped on content (line 181)
- Close button with proper ARIA label (line 175)
- Synchronized
initialSlidefrom main gallery (line 190)- Proper null checking for thumbs Swiper (line 194)
The implementation provides a smooth fullscreen viewing experience with keyboard navigation support.
47-49: No action needed—slideToLoop()is the correct Swiper API method for this use case.The Swiper v12 API documentation confirms that
slideToLoop(index, speed)is the standard method specifically designed for loop mode navigation. It jumps to the slide whose realIndex matches the passed index and handles duplicated looped slides automatically. The second parameter (0) correctly specifies instant navigation (0ms duration). This is the appropriate method choice here and no changes are required.Likely an incorrect or invalid review comment.
packages/blocks/product-details/src/api-harmonization/product-details.model.ts
Show resolved
Hide resolved
packages/blocks/product-details/src/api-harmonization/product-details.service.ts
Outdated
Show resolved
Hide resolved
packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx
Show resolved
Hide resolved
packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (1)
40-42: Accessibility concern: Duplicate<h1>elements still present.This mobile
<h1>(line 41) and the desktop<h1>(line 140) both exist in the DOM simultaneously, violating accessibility guidelines. Consider using a single<h1>with responsive styling, or applyingaria-hidden="true"to the visually hidden duplicate.
🧹 Nitpick comments (5)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (5)
113-133: Consider making icon names configurable.The icon names "FileText" (line 119) and "MapPin" (line 127) are hardcoded. If these may need to vary by tenant or configuration, consider moving them to the labels or configuration object.
166-199: Consider extracting action button rendering to reduce duplication.The action button rendering logic (href vs onClick, icon handling) is duplicated between the desktop sidebar (lines 170-196) and mobile footer (lines 227-244). Consider extracting this into a helper component or render function.
🔎 Example refactor approach
Create a helper function above the component return:
const renderActionButton = (mobile = false) => { if (!actionButton) return null; const buttonSize = mobile ? 'default' : 'lg'; const showIcon = !mobile && actionButton.icon; const buttonContent = ( <> {showIcon && <DynamicIcon name={actionButton.icon} size={20} className="mr-2" />} {actionButton.label} </> ); const buttonProps = { variant: actionButton.variant || 'default', size: buttonSize, className: 'w-full', }; if (actionButton.href) { return ( <Button {...buttonProps} asChild> <LinkComponent href={actionButton.href}> {buttonContent} </LinkComponent> </Button> ); } return ( <Button {...buttonProps} onClick={actionButton.onClick}> {buttonContent} </Button> ); };Then use
{renderActionButton()}in desktop and{renderActionButton(true)}in mobile.
204-215: TODO: Recommended products section is incomplete.The popular offers section is scaffolded but shows a TODO placeholder (line 212). The
popularOffersdata is available but not rendered.Would you like me to generate an implementation for the recommended products section, or should I open an issue to track this task?
220-220: Asymmetric margin on mobile footer content.The footer content uses
ml-auto mr-4, creating asymmetric horizontal margins. If this is intentional for visual alignment, consider adding a comment. Otherwise, consider usingmx-autoor symmetric padding.
249-249: Magic number: Spacer height should match footer height.The spacer uses
h-32(8rem/128px) to prevent content from being hidden by the fixed footer. Consider using a CSS variable or constant that matches the actual footer height to maintain consistency if the footer size changes.🔎 Potential approach
Define a constant at the top of the file:
const MOBILE_FOOTER_HEIGHT = 'h-32'; // or 128px if using inline stylesThen reference it for both the spacer calculation and potentially for calculating the footer's actual height, or document why h-32 was chosen.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (5)
packages/blocks/product-details/src/frontend/ProductDetails.types.ts (1)
ProductDetailsPureProps(14-14)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
Badge(5-8)packages/ui/src/components/DynamicIcon/DynamicIcon.tsx (1)
DynamicIcon(8-42)packages/ui/src/components/RichText/RichText.tsx (1)
RichText(48-252)packages/framework/src/utils/models/price.ts (1)
Price(3-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (3)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (3)
62-81: LGTM: Key specs section.The conditional rendering, responsive grid layout, and optional icon/value handling are well-implemented. The DynamicIcon component properly handles missing icons by returning null.
83-111: LGTM: Description and specifications sections.Both sections use proper conditional rendering, semantic HTML headings, and responsive layouts. The RichText component appropriately handles the description content.
159-164: Price component already gracefully handles missing or invalid price data.The Price component includes explicit null/undefined handling (returns
nullwhen price is falsy), properly types the price prop as optional, and the usePriceService includes fallback mechanisms for edge cases like unknown currencies. No changes are needed.
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (1)
66-68: Consider: className relies on Swiper internal DOM structure.The className uses deeply nested Swiper-specific selectors (
[&_.swiper-slide],[&_.swiper-pagination-bullet], etc.) which couples the styling to Swiper's internal DOM structure. If the Carousel component changes its underlying implementation or Swiper updates its markup, these selectors may break.Consider either:
- Exposing styling props on the Carousel component itself for better encapsulation
- Documenting this dependency on Swiper's DOM structure
- Adding integration tests to catch breaking changes in the Carousel/Swiper structure
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/ui/src/components/ProductCarousel/ProductCarousel.stories.tsxpackages/ui/src/components/ProductCarousel/ProductCarousel.tsxpackages/ui/src/components/ProductCarousel/ProductCarousel.types.tspackages/ui/src/components/ProductCarousel/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui/src/components/ProductCarousel/ProductCarousel.stories.tsx (2)
packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (1)
ProductCarousel(15-88)packages/ui/src/components/ProductCarousel/ProductCarousel.types.ts (1)
ProductSummaryItem(19-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (6)
packages/ui/src/components/ProductCarousel/index.ts (1)
1-2: LGTM! Clean barrel exports.The index file follows standard patterns by re-exporting both the component and its public types, enabling consumers to import from a single entry point.
packages/ui/src/components/ProductCarousel/ProductCarousel.stories.tsx (1)
11-16: LGTM! MockLinkComponent is appropriate for stories.The mock implementation correctly satisfies the LinkComponent interface for Storybook demonstration purposes.
packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (2)
25-27: LGTM! Appropriate early return for edge case.The guard clause correctly handles the empty products scenario, preventing unnecessary rendering.
29-42: LGTM! Clean conditional rendering logic.The header section renders appropriately based on the presence of title, description, or action, with responsive layout support.
packages/ui/src/components/ProductCarousel/ProductCarousel.types.ts (2)
19-27: LGTM! Well-structured ProductSummaryItem interface.The interface appropriately defines required fields (id, name, link) and optional fields (description, image, price, badges) for product summary data.
29-32: LGTM! ProductBadge interface is clean and well-typed.The variant options align with common UI badge patterns and the interface is appropriately simple.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx (1)
16-16: Remove__typenamefrom story args (duplicate concern).As previously flagged, the
__typenamefield should not be included in story args for a UI component. Based on learnings,__typenameis a discriminator used at the API harmonization layer, not in frontend component props.🔎 Suggested fix
export const Default: Story = { args: { - __typename: 'ProductDetailsBlock', id: 'PRD-015',Based on learnings,
__typenamediscriminators are added at the API harmonization layer, not in framework or frontend UI components.packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (1)
84-84: Reminder: carouselConfig can still override critical props.This issue was previously flagged but remains unfixed. The
carouselConfigspread after critical props allows users to overrideslides,slidesPerView,breakpoints, and other essential configuration, potentially breaking the component.Please refer to the previous review comment on lines 45-85 for detailed solutions (either reorder the spread or restrict the type with
Omit).
🧹 Nitpick comments (2)
packages/blocks/product-details/src/api-harmonization/product-details.service.spec.ts (1)
52-62: Consider adding behavioral tests beyond existence checks.The current tests only verify that the service and its dependencies are defined. While this confirms the module wiring is correct, it doesn't validate the actual behavior of
ProductDetailsService.Consider adding tests that:
- Call the
getProductDetailsmethod with sample inputs- Verify that CMS and Products services are invoked with correct parameters
- Test error scenarios (e.g., product not found, CMS service failures)
- Validate data transformation and mapping logic
Would you like me to generate sample test cases for the
getProductDetailsmethod?packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (1)
39-45: Hardcoded actionButton properties.The
actionButtonuses hardcoded values forvariant('default') andicon('MessageCircle'). While this may be intentional for consistency, consider whether these should be configurable via the CMS to support different use cases or localization needs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/blocks/product-details/src/api-harmonization/product-details.mapper.tspackages/blocks/product-details/src/api-harmonization/product-details.model.tspackages/blocks/product-details/src/api-harmonization/product-details.service.spec.tspackages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsxpackages/blocks/product-details/src/frontend/ProductDetails.client.tsxpackages/framework/src/modules/cms/models/blocks/product-details.model.tspackages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/ui/src/components/ProductCarousel/ProductCarousel.stories.tsxpackages/ui/src/components/ProductCarousel/ProductCarousel.tsxpackages/ui/src/components/ProductCarousel/ProductCarousel.types.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts
- packages/framework/src/modules/cms/models/blocks/product-details.model.ts
- packages/ui/src/components/ProductCarousel/ProductCarousel.types.ts
- packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts
- packages/ui/src/components/ProductCarousel/ProductCarousel.stories.tsx
- packages/blocks/product-details/src/frontend/ProductDetails.client.tsx
- packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts
- packages/blocks/product-details/src/api-harmonization/product-details.model.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-26T11:57:00.632Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
Applied to files:
packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsxpackages/blocks/product-details/src/api-harmonization/product-details.mapper.ts
📚 Learning: 2025-11-28T14:07:04.474Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/blocks/product-list/src/frontend/ProductList.client.stories.tsx:15-18
Timestamp: 2025-11-28T14:07:04.474Z
Learning: Mock JWT tokens with placeholder data (e.g., Jane Doe, Acme Corporation) used in Storybook story files (.stories.tsx) for demo/test purposes are acceptable and do not need to be replaced with non-JWT placeholders.
Applied to files:
packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx
🧬 Code graph analysis (2)
packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (4)
packages/ui/src/components/ProductCarousel/ProductCarousel.types.ts (1)
ProductCarouselProps(8-17)packages/ui/src/components/RichText/RichText.tsx (1)
RichText(48-252)packages/ui/src/components/Cards/ProductCard/ProductCard.tsx (1)
ProductCard(16-94)packages/ui/src/lib/utils.ts (1)
cn(5-7)
packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (4)
packages/framework/src/modules/products/products.model.ts (2)
Products(43-43)Product(19-41)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (5)
Product(10-15)ProductSummary(17-25)ProductDetailsBlock(46-52)Badge(5-8)Labels(35-44)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (2)
ProductDetailsBlock(14-17)Labels(3-12)apps/api-harmonization/src/modules/page/page.model.ts (1)
Labels(43-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: deploy-preview
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
packages/blocks/product-details/src/api-harmonization/product-details.service.spec.ts (2)
1-6: LGTM! Imports are appropriate.The imports are well-organized and include all necessary testing utilities for NestJS with Vitest.
8-50: Test setup is well-structured.The testing module configuration properly mocks the CMS.Service and Products.Service dependencies. The mock data for
getProductDetailsBlockincludes comprehensive label properties.Note: The
Products.Servicemocks (getProduct,getProductList) currently have no return values. If you add behavioral tests that invoke these methods, you'll need to define appropriate return values using.mockReturnValue()or.mockResolvedValue().packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx (3)
1-12: LGTM! Standard Storybook setup.The imports and meta configuration follow Storybook best practices with proper TypeScript typing.
203-215: LGTM! Good story variant demonstrating secondary button.The story correctly extends the Default args and demonstrates the secondary button variant. The console.log in the onClick handler is appropriate for Storybook demonstrations.
217-222: LGTM! Good story variant demonstrating optional button state.The story correctly demonstrates the case where no action button is provided by explicitly setting it to
undefined.packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (1)
1-27: LGTM - Clean component setup and guard clause.The imports, prop destructuring, and early return for empty products are well implemented.
packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (2)
1-9: LGTM!The imports and function signature are well-structured and correctly typed.
23-32: No issue found. The CMS model's type system guarantees required fields.The
Labelstype defines four required fields, and the mapper correctly assumes they're provided. The CMS model declarationlabels!: Labels(with definite assignment assertion) guarantees that thelabelsproperty always contains a fully formedLabelsobject with all required fields populated. Direct field access without null checks is appropriate here and is enforced by TypeScript's type system.
packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/blocks/product-details/src/frontend/ProductDetails.renderer.tsx:
- Around line 10-15: ProductDetailsRenderer is not destructuring the optional
cmsBlockId from ProductDetailsRendererProps, so any passed cmsBlockId is
ignored; update the parameter destructuring of the ProductDetailsRenderer
component to include cmsBlockId and then forward or use that value where the
component renders or assembles props (e.g., pass cmsBlockId into child
components or API calls that previously received id/routing/locale), ensuring
the prop name exactly matches cmsBlockId used in ProductDetails.types.ts.
- Line 31: The ProductDetails.renderer is not forwarding the cmsBlockId prop to
the ProductDetails server component, causing it to always use the default
'product-details-1'; update the renderer to accept cmsBlockId from its props and
pass cmsBlockId into the JSX <ProductDetails ... />; specifically modify the
ProductDetails.renderer component to include cmsBlockId in its props signature
and add cmsBlockId={cmsBlockId} to the ProductDetails invocation so the server
component (ProductDetails) receives the custom ID.
🧹 Nitpick comments (3)
packages/blocks/product-details/src/api-harmonization/product-details.service.ts (3)
23-23: Consider extracting the default locale to a constant.The hardcoded
'en'default could be moved to a configuration constant for easier maintenance.♻️ Suggested refactor
Define a constant at the top of the file or in a shared config:
+const DEFAULT_LOCALE = 'en'; + @Injectable() export class ProductDetailsService {Then use it in the locale resolution:
- const locale = query.locale || headers['x-locale'] || 'en'; + const locale = query.locale || headers['x-locale'] || DEFAULT_LOCALE;
33-37: Consider requesting more than 4 popular offers to account for filtering.Since the popular offers are filtered on lines 46-48 to exclude the current product and offers without images, requesting only 4 items may result in fewer than 4 final results. Requesting 5-6 items would help ensure you have 4 offers after filtering.
♻️ Suggested improvement
const popularOffers = this.productsService.getProductList({ - limit: 4, + limit: 6, // Request extra to account for filtering offset: 0, locale, });
53-53: Non-null assertion is safe here but consider optional chaining.The non-null assertion
offer.image!is safe because line 47 filters out offers without images. However, using optional chaining would be more defensive.♻️ More defensive approach
description: offer.shortDescription, - image: offer.image!, + image: offer.image ?? { url: '', alt: '' }, // Or handle appropriately price: offer.price,Note: This assumes a reasonable default or that you handle the case differently. The current code is correct given the filter.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/blocks/product-details/src/api-harmonization/product-details.request.tspackages/blocks/product-details/src/api-harmonization/product-details.service.tspackages/blocks/product-details/src/frontend/ProductDetails.renderer.tsxpackages/blocks/product-details/src/frontend/ProductDetails.server.tsxpackages/blocks/product-details/src/frontend/ProductDetails.types.tspackages/ui/src/components/ProductGallery/ProductGallery.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/ui/src/components/ProductGallery/ProductGallery.tsx
- packages/blocks/product-details/src/api-harmonization/product-details.request.ts
- packages/blocks/product-details/src/frontend/ProductDetails.server.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
📚 Learning: 2025-11-26T11:57:00.632Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
Applied to files:
packages/blocks/product-details/src/frontend/ProductDetails.types.ts
🧬 Code graph analysis (3)
packages/blocks/product-details/src/api-harmonization/product-details.service.ts (4)
packages/blocks/product-details/src/api-harmonization/product-details.request.ts (1)
GetProductDetailsBlockQuery(5-8)packages/framework/src/utils/models/headers.ts (1)
AppHeaders(1-6)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (3)
ProductDetailsBlock(46-52)ProductSummary(17-25)Badge(5-8)packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (1)
mapProductDetails(5-48)
packages/blocks/product-details/src/frontend/ProductDetails.types.ts (2)
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
ProductDetailsBlock(46-52)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
ProductDetailsBlock(14-17)
packages/blocks/product-details/src/frontend/ProductDetails.renderer.tsx (5)
packages/blocks/product-details/src/frontend/index.ts (2)
ProductDetailsRenderer(3-3)ProductDetails(2-2)packages/blocks/product-details/src/frontend/ProductDetails.types.ts (1)
ProductDetailsRendererProps(15-21)packages/ui/src/components/Container/Container.tsx (1)
Container(42-62)packages/blocks/product-details/src/frontend/ProductDetails.server.tsx (1)
ProductDetails(12-27)apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (4)
packages/blocks/product-details/src/frontend/ProductDetails.types.ts (1)
1-21: LGTM!The type definitions are well-structured and follow consistent patterns:
- Clear separation between server props (locale required) and renderer props (locale optional with hook fallback)
- Appropriate use of intersection types for
ProductDetailsPureProps- Consistent optional field definitions across the type hierarchy
packages/blocks/product-details/src/api-harmonization/product-details.service.ts (3)
1-16: LGTM! Clean service structure.The imports, dependency injection, and NestJS decorators are all properly configured.
62-62: LGTM! Clean delegation to mapper.The final mapping call properly assembles the ProductDetailsBlock using the filtered data.
56-59: Runtime validation needed for badge variant values.This is the same type-safety concern raised in the previous review. The cast
tag.variant as Model.Badge['variant']bypasses TypeScript checking. If the Products API returns an unexpected variant value, it could cause rendering issues downstream.The previous review recommended implementing runtime validation or a type guard before casting. Consider validating
tag.variantagainst the allowed Badge variants ('default' | 'secondary' | 'destructive' | 'outline') and using a fallback if invalid.Likely an incorrect or invalid review comment.
packages/blocks/product-details/src/frontend/ProductDetails.renderer.tsx
Show resolved
Hide resolved
packages/blocks/product-details/src/frontend/ProductDetails.renderer.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/ui/src/components/ProductGallery/ProductGallery.tsx:
- Around line 62-80: The effect that disables body scroll when the lightbox
opens should preserve and restore the body's original overflow value: inside the
useEffect (the block that checks isLightboxOpen and defines handleEscKey in
ProductGallery), capture const previousOverflow = document.body.style.overflow
before setting document.body.style.overflow = 'hidden', and on cleanup restore
document.body.style.overflow = previousOverflow; keep the existing
addEventListener/removeEventListener logic around handleEscKey and keep
dependencies [isLightboxOpen, handleCloseLightbox].
🧹 Nitpick comments (2)
packages/ui/src/components/ProductGallery/ProductGallery.tsx (2)
47-49: Consider memoizing handleThumbnailHover for consistency.While the function is small and the optional chaining makes it safe, wrapping it in
useCallbackwould be consistent with the other handlers and prevent unnecessary re-renders of child components.♻️ Optional refactor
-const handleThumbnailHover = (index: number, swiperInstance: SwiperType | null) => { - swiperInstance?.slideToLoop(index, 0); -}; +const handleThumbnailHover = useCallback((index: number, swiperInstance: SwiperType | null) => { + swiperInstance?.slideToLoop(index, 0); +}, []);
87-128: Conditionally disable loop mode for single image.The main gallery always uses
loop={true}(line 103), which can cause unexpected behavior when there's only one image. Swiper's loop mode creates duplicate slides, resulting in odd navigation behavior for single images.♻️ Recommended fix to conditionally enable loop
<Swiper className={cn( 'w-full rounded-lg overflow-hidden', showNavigation && showThumbnails && '[&_.swiper-button-next]:opacity-0 [&_.swiper-button-prev]:opacity-0 [&_.swiper-button-next]:transition-opacity [&_.swiper-button-prev]:transition-opacity group-hover:[&_.swiper-button-next]:opacity-100 group-hover:[&_.swiper-button-prev]:opacity-100', )} modules={mainModules} keyboard={{ enabled: true, onlyInViewport: true }} navigation={showNavigation} pagination={showPagination ? { clickable: true } : false} speed={speed} - loop={true} + loop={images.length > 1} thumbs={{ swiper: showThumbnails && thumbsSwiper && !thumbsSwiper?.destroyed ? thumbsSwiper : null, }} onSwiper={setMainSwiper} {...swiperProps} >Apply the same change to the lightbox Swiper at line 189.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsxpackages/ui/src/components/ProductGallery/ProductGallery.tsxpackages/ui/src/components/ProductGallery/ProductGallery.types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/src/components/ProductGallery/ProductGallery.types.ts
- packages/blocks/product-details/src/frontend/ProductDetails.client.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
🧬 Code graph analysis (1)
packages/ui/src/components/ProductGallery/ProductGallery.tsx (2)
packages/ui/src/components/ProductGallery/ProductGallery.types.ts (1)
ProductGalleryProps(8-16)packages/ui/src/lib/utils.ts (1)
cn(5-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: deploy-preview
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
packages/ui/src/components/ProductGallery/ProductGallery.tsx (7)
1-28: LGTM: Clean component setup.The imports, type annotations, and default prop values are well-structured. The component signature properly extends SwiperProps and provides sensible defaults.
29-34: LGTM: State management is clear and well-typed.The separate useState hooks for different Swiper instances and lightbox state provide good clarity and are appropriately typed.
36-45: LGTM: Proper memoization of Swiper modules.The conditional module array is correctly memoized with appropriate dependencies, avoiding unnecessary re-renders.
82-85: LGTM: Good defensive check.The early return for empty or missing images prevents unnecessary rendering and potential errors.
165-178: LGTM: Well-structured and accessible lightbox.The lightbox implementation includes proper accessibility features (aria-label, ESC key support), backdrop click-to-close with content click protection, and a clear close button.
183-212: No action needed—the code correctly handles initialSlide with loop mode.The lightbox Swiper correctly captures the current slide index using
mainSwiper.realIndex(which returns the logical slide index accounting for loop duplication) and passes it to the lightbox'sinitialSlideprop. Swiper's documentation confirms thatinitialSlideworks correctly withloop={true}when provided with the logical slide index, and this code already implements that pattern. No misalignment will occur at boundaries.
144-144: No action needed. Thew-auto!syntax is correct for Tailwind CSS v4, which this project uses (as specified inpackages/ui/package.json). In Tailwind v4, the important modifier uses postfix syntax (w-auto!), unlike v3 which used prefix syntax (!w-auto). The code is valid.Likely an incorrect or invalid review comment.
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx
Outdated
Show resolved
Hide resolved
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx
Outdated
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/integrations/mocked/src/modules/cms/mappers/cms.page.mapper.ts
Dismissed
Show dismissed
Hide dismissed
packages/integrations/mocked/src/modules/cms/mappers/cms.page.mapper.ts
Dismissed
Show dismissed
Hide dismissed
packages/integrations/mocked/src/modules/cms/mappers/cms.page.mapper.ts
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/integrations/mocked/src/modules/cms/mappers/cms.page.mapper.ts (1)
365-424: Missing product details pages ingetAlternativePages.The new
PAGE_PRODUCT_DETAILS_EN/DE/PLpages are not included ingetAlternativePages, while other detail pages likePAGE_SERVICE_DETAILS_*are (lines 394-396). This will cause alternative page lookups to fail for product details.Suggested fix
PAGE_SERVICE_DETAILS_EN, PAGE_SERVICE_DETAILS_DE, PAGE_SERVICE_DETAILS_PL, + PAGE_PRODUCT_DETAILS_EN, + PAGE_PRODUCT_DETAILS_DE, + PAGE_PRODUCT_DETAILS_PL, PAGE_CONTACT_US_EN,
🤖 Fix all issues with AI agents
In @packages/blocks/product-details/src/frontend/ProductDetails.client.tsx:
- Line 202: The div in ProductDetails.client.tsx uses a non-standard Tailwind
class "z-100"; replace it with a valid Tailwind value (either downscale to an
existing class like "z-50" or use Tailwind arbitrary value syntax "z-[100]") so
the element gets the intended stacking context; update the className on the
fixed bottom div (the line containing "lg:hidden fixed bottom-0 left-0 right-0
bg-background border-t border-border p-4 shadow-lg z-100") to use "z-[100]" (or
"z-50" if that suffices) to fix the invalid class.
In @packages/blocks/recommended-products/package.json:
- Line 6: The package.json description string is grammatically incorrect ("an
RecommendedProducts"); update the "description" field for the
packages/blocks/recommended-products package to use correct phrasing such as "A
simple block displaying static content in the form of a Recommended Products
block" (or similar) so it reads naturally and uses "a" before "Recommended
Products block."
In
@packages/blocks/recommended-products/src/api-harmonization/recommended-products.service.ts:
- Around line 53-56: The mapping for badges (in the badges:
product.tags?.map(...) block) unsafely casts tag.variant to
Model.Badge['variant']; replace this with a validation/guard that checks
tag.variant against the allowed union ('default' | 'secondary' | 'destructive' |
'outline') and use a safe fallback (e.g., 'default') if it is not valid.
Implement a small helper or inline whitelist check (e.g., isValidBadgeVariant)
and use it when constructing the badge variant instead of the direct type
assertion to ensure only permitted values are used at runtime.
- Around line 29-45: The current flow calls productsService.getProductList with
limit and then filters out excluded or image-less items in the mapping step (see
productsService.getProductList and the filtering block that produces
filteredProducts), which can yield fewer than the requested limit; fix by
fetching a larger initial page or implementing iterative fetches until you
accumulate `limit` valid items: modify the call to getProductList to request a
higher buffer (e.g., limit * 2) or implement a paging loop/ RxJS expand using
offset to fetch additional pages from productsService.getProductList and keep
appending filtered results until you have `limit` items (then slice to limit),
ensuring the excludeProductId and image check are applied on each batch before
counting toward the final result.
In
@packages/blocks/recommended-products/src/frontend/RecommendedProducts.renderer.tsx:
- Around line 21-22: The code assigns productIdToExclude using slug[1] without
validating slug; update the logic in RecommendedProducts.renderer.tsx (the
productIdToExclude assignment that references excludeProductId and slug) to
first verify slug is defined and has length > 1 before reading slug[1],
otherwise fall back to undefined or excludeProductId; implement this by
replacing the direct access with a safe conditional (e.g., use a ternary that
checks slug && slug.length > 1) so you never access slug[1] when slug is missing
or too short.
In @packages/blocks/recommended-products/src/sdk/index.ts:
- Around line 9-14: API_URL is force-unwrapped when passed to getSdk as apiUrl
(internalSdk creation), which can throw at runtime if environment vars are
missing; validate that API_URL is defined before using it (or provide a safe
fallback), and if undefined throw or log a clear, early error explaining the
missing NEXT_PUBLIC_API_URL/_INTERNAL var so internalSdk is never constructed
with a null/undefined apiUrl. Ensure the check runs before calling getSdk and
reference API_URL and internalSdk/getSdk in the fix.
In
@packages/framework/src/modules/cms/models/blocks/recommended-products.model.ts:
- Around line 3-8: Define a dedicated Labels type and make the property required
to match the ProductDetailsBlock pattern: add an exported type (e.g., export
type Labels = { title?: string; detailsLabel?: string; }) and change the
RecommendedProductsBlock.labels declaration from an inline optional type to a
required field using that type (labels!: Labels) while keeping the same keys;
reference ProductDetailsBlock for consistency.
In
@packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts:
- Line 7: The mapper currently emits a hardcoded id value ('product-details-1')
which can collide for multiple instances; update the cms.product-details.mapper
to generate a unique identifier per mapped block (use the Contentful
entry/system id if available, e.g. source.sys.id or a deterministic key derived
from entry data, or fallback to a UUID/v4) instead of the hardcoded
'product-details-1' so each mapped product details block has a distinct id;
replace the literal 'product-details-1' reference with this generated value and
ensure any TODO that maps Contentful fields uses that unique id.
🧹 Nitpick comments (10)
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.recommended-products.mapper.ts (1)
3-25: Minor localization inconsistency in Polish mock.The Polish mock (line 23) has
detailsLabel: 'Szczegóły'while EN and DE both use'Details'. For consistency, consider whether the German version should also be localized (e.g.,'Details'→'Einzelheiten'or keeping English is intentional).♻️ Optional: Localize German label
const MOCK_RECOMMENDED_PRODUCTS_BLOCK_DE: CMS.Model.RecommendedProductsBlock.RecommendedProductsBlock = { id: 'recommended-products-1', labels: { title: 'Empfohlene Produkte', - detailsLabel: 'Details', + detailsLabel: 'Einzelheiten', }, };packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.recommended-products.mapper.ts (1)
3-15: TODO: Complete Contentful integration and locale handling.The mapper is currently a stub with hardcoded English labels that ignores the
localeparameter. The TODO on line 6 flags this, but ensure this is tracked for completion.Would you like me to:
- Open a tracking issue for completing the Contentful integration?
- Generate a proper implementation based on the mocked integration pattern (lines 27-40 in
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.recommended-products.mapper.ts)?Note: The same TODO exists in the Strapi integration (
packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.recommended-products.mapper.ts).packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
3-17: LGTM! Stub implementation follows established pattern.The mapper structure is consistent with other CMS integration stubs (Strapi, mocked). The TODO clearly indicates that proper Contentful mapping is pending implementation.
Would you like me to open an issue to track the implementation of proper Contentful mapping for this block?
packages/blocks/recommended-products/src/api-harmonization/recommended-products.request.ts (1)
3-7: Consider removing redundantiddeclaration.The
id!: stringfield on Line 4 is redundant since it's already declared inCMS.Request.GetCmsEntryParams. While this doesn't cause issues, removing it would make the class leaner and clarify that onlyexcludeProductIdandlimitare additions specific to this query.♻️ Proposed refactor
export class GetRecommendedProductsBlockQuery implements Omit<CMS.Request.GetCmsEntryParams, 'locale'> { - id!: string; excludeProductId?: string; limit?: number; }packages/blocks/recommended-products/src/sdk/recommended-products.ts (1)
18-18: Simplify URL assignment.The template literal interpolation
\${URL}`is unnecessary sinceURL` is already a string. Assigning it directly would be cleaner.♻️ Proposed refactor
- url: `${URL}`, + url: URL,packages/blocks/recommended-products/src/api-harmonization/recommended-products.service.spec.ts (1)
7-37: Expand test coverage beyond dependency injection checks.The current tests only verify that the service and its dependency are defined, which provides minimal coverage. Consider adding tests that verify actual service behavior, such as:
- Calling methods on the service and asserting their return values
- Verifying that the mocked
getRecommendedProductsBlockis called with expected parameters- Testing error handling scenarios
📝 Example of enhanced test coverage
+ it('should fetch recommended products block from CMS', async () => { + const mockBlock = { title: 'Test Block' }; + vi.spyOn(cmsService, 'getRecommendedProductsBlock').mockReturnValue(mockBlock); + + const result = await service.getBlock({ id: 'test-id', locale: 'en' }); + + expect(cmsService.getRecommendedProductsBlock).toHaveBeenCalledWith({ + id: 'test-id', + locale: 'en' + }); + expect(result).toEqual(mockBlock); + });packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.recommended-products.mapper.ts (1)
3-15: Placeholder implementation noted—proper Strapi mapping pending.The TODO comment correctly indicates that this is a placeholder returning hardcoded values. The mapper will need to implement proper Strapi-to-CMS model transformation.
Would you like me to open a tracking issue for implementing the proper Strapi mapping, or provide a template based on similar mappers in the codebase (e.g.,
cms.product-details.mapper.ts)?packages/blocks/recommended-products/src/frontend/RecommendedProducts.client.tsx (1)
14-15: Optional: Simplify the conditional check.The check
!products || products.length === 0can be simplified to just!products?.length, which handles both null/undefined and empty array cases.♻️ Proposed refactor
- if (!products || products.length === 0) { + if (!products?.length) { return null; }packages/blocks/recommended-products/src/frontend/RecommendedProducts.server.tsx (2)
8-10: Consider improving type safety of the dynamic component.The dynamic import resolves to
React.ComponentType<any>, which loses type safety. Since you're passing specific props toRecommendedProductsDynamic(lines 30-37), consider explicitly typing it based on the client component's props interface.♻️ Suggested improvement
If
RecommendedProductsPureexports a props type (e.g.,RecommendedProductsPureProps), you can type the dynamic import:import type { RecommendedProductsPureProps } from './RecommendedProducts.client'; export const RecommendedProductsDynamic = dynamic<RecommendedProductsPureProps>(() => import('./RecommendedProducts.client').then((module) => module.RecommendedProductsPure), );
39-42: Consider adding a fallback UI for better error visibility.Returning
nullon error provides silent failure, which may not be ideal for user experience. Users won't see any indication that the recommended products block failed to load.💡 Suggested improvement
Consider returning a minimal error message or empty state instead of
null:} catch (error) { console.error('Error fetching RecommendedProducts block', error); - return null; + return ( + <div role="alert" className="text-sm text-muted-foreground"> + Unable to load recommended products + </div> + ); }Alternatively, leverage React's error boundary pattern if consistent error handling is needed across multiple blocks.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (58)
apps/api-harmonization/package.jsonapps/api-harmonization/src/app.module.tsapps/api-harmonization/src/modules/page/page.model.tsapps/frontend/package.jsonapps/frontend/src/blocks/renderBlocks.tsxpackages/blocks/product-details/src/api-harmonization/product-details.mapper.tspackages/blocks/product-details/src/api-harmonization/product-details.model.tspackages/blocks/product-details/src/api-harmonization/product-details.service.spec.tspackages/blocks/product-details/src/api-harmonization/product-details.service.tspackages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsxpackages/blocks/product-details/src/frontend/ProductDetails.client.tsxpackages/blocks/product-details/src/frontend/ProductDetails.renderer.tsxpackages/blocks/product-details/src/frontend/ProductDetails.server.tsxpackages/blocks/product-details/src/frontend/ProductDetails.types.tspackages/blocks/recommended-products/.gitignorepackages/blocks/recommended-products/.prettierrc.mjspackages/blocks/recommended-products/eslint.config.mjspackages/blocks/recommended-products/lint-staged.config.mjspackages/blocks/recommended-products/package.jsonpackages/blocks/recommended-products/src/api-harmonization/index.tspackages/blocks/recommended-products/src/api-harmonization/recommended-products.client.tspackages/blocks/recommended-products/src/api-harmonization/recommended-products.controller.tspackages/blocks/recommended-products/src/api-harmonization/recommended-products.mapper.tspackages/blocks/recommended-products/src/api-harmonization/recommended-products.model.tspackages/blocks/recommended-products/src/api-harmonization/recommended-products.module.tspackages/blocks/recommended-products/src/api-harmonization/recommended-products.request.tspackages/blocks/recommended-products/src/api-harmonization/recommended-products.service.spec.tspackages/blocks/recommended-products/src/api-harmonization/recommended-products.service.tspackages/blocks/recommended-products/src/frontend/RecommendedProducts.client.stories.tsxpackages/blocks/recommended-products/src/frontend/RecommendedProducts.client.tsxpackages/blocks/recommended-products/src/frontend/RecommendedProducts.renderer.tsxpackages/blocks/recommended-products/src/frontend/RecommendedProducts.server.tsxpackages/blocks/recommended-products/src/frontend/RecommendedProducts.types.tspackages/blocks/recommended-products/src/frontend/index.tspackages/blocks/recommended-products/src/sdk/index.tspackages/blocks/recommended-products/src/sdk/recommended-products.tspackages/blocks/recommended-products/tsconfig.api.jsonpackages/blocks/recommended-products/tsconfig.frontend.jsonpackages/blocks/recommended-products/tsconfig.jsonpackages/blocks/recommended-products/tsconfig.sdk.jsonpackages/blocks/recommended-products/vitest.config.mjspackages/framework/src/modules/cms/cms.model.tspackages/framework/src/modules/cms/cms.service.tspackages/framework/src/modules/cms/models/blocks/product-details.model.tspackages/framework/src/modules/cms/models/blocks/recommended-products.model.tspackages/integrations/contentful-cms/src/modules/cms/cms.service.tspackages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.recommended-products.mapper.tspackages/integrations/mocked/src/modules/cms/cms.service.tspackages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/mocked/src/modules/cms/mappers/blocks/cms.recommended-products.mapper.tspackages/integrations/mocked/src/modules/cms/mappers/cms.page.mapper.tspackages/integrations/mocked/src/modules/cms/mappers/mocks/pages/product-details.page.tspackages/integrations/mocked/src/modules/products/products.mapper.tspackages/integrations/mocked/src/modules/resources/mock/products.mock.tspackages/integrations/strapi-cms/src/modules/cms/cms.service.tspackages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.recommended-products.mapper.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/integrations/contentful-cms/src/modules/cms/cms.service.ts
- packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts
- packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts
- packages/blocks/product-details/src/api-harmonization/product-details.service.ts
- packages/framework/src/modules/cms/models/blocks/product-details.model.ts
- packages/integrations/strapi-cms/src/modules/cms/cms.service.ts
- apps/api-harmonization/src/app.module.ts
- apps/frontend/package.json
- apps/api-harmonization/package.json
- packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts
- packages/integrations/mocked/src/modules/cms/cms.service.ts
- packages/blocks/product-details/src/api-harmonization/product-details.service.spec.ts
- packages/blocks/product-details/src/api-harmonization/product-details.model.ts
- packages/integrations/mocked/src/modules/products/products.mapper.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-26T11:57:00.632Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
Applied to files:
packages/framework/src/modules/cms/models/blocks/recommended-products.model.tspackages/blocks/recommended-products/tsconfig.jsonpackages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.recommended-products.mapper.tspackages/framework/src/modules/cms/cms.model.tspackages/blocks/recommended-products/src/api-harmonization/recommended-products.request.tspackages/blocks/recommended-products/src/api-harmonization/recommended-products.client.tspackages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.recommended-products.mapper.tspackages/blocks/recommended-products/src/frontend/RecommendedProducts.types.tspackages/framework/src/modules/cms/cms.service.tspackages/blocks/product-details/src/frontend/ProductDetails.types.tspackages/blocks/recommended-products/src/api-harmonization/recommended-products.module.tspackages/blocks/recommended-products/tsconfig.sdk.jsonapps/api-harmonization/src/modules/page/page.model.tspackages/blocks/recommended-products/src/api-harmonization/recommended-products.mapper.tspackages/blocks/product-details/src/frontend/ProductDetails.renderer.tsxpackages/blocks/recommended-products/tsconfig.api.jsonpackages/blocks/recommended-products/src/api-harmonization/recommended-products.model.tspackages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsxpackages/blocks/recommended-products/package.jsonpackages/integrations/mocked/src/modules/cms/mappers/blocks/cms.recommended-products.mapper.tsapps/frontend/src/blocks/renderBlocks.tsxpackages/blocks/recommended-products/tsconfig.frontend.jsonpackages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts
📚 Learning: 2025-11-14T14:34:25.489Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 357
File: packages/blocks/ticket-summary/eslint.config.mjs:1-18
Timestamp: 2025-11-14T14:34:25.489Z
Learning: ESLint v9+ provides `defineConfig` as an export from 'eslint/config' module, which supports an `extends` array property in flat config objects for composing configurations.
Applied to files:
packages/blocks/recommended-products/eslint.config.mjspackages/blocks/recommended-products/lint-staged.config.mjspackages/blocks/recommended-products/.prettierrc.mjs
📚 Learning: 2025-11-14T14:24:34.907Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 357
File: packages/blocks/ticket-summary/src/frontend/TicketSummary.types.ts:14-16
Timestamp: 2025-11-14T14:24:34.907Z
Learning: In the openselfservice repository, the pattern `Omit<PropsType, ''>` in TypeScript type definitions is intentionally used as a template for future prop omissions, even though it currently does nothing. This convention is preferred for consistency across blocks, making it easier to add omissions when needed later.
Applied to files:
packages/blocks/recommended-products/src/frontend/RecommendedProducts.types.tspackages/blocks/product-details/src/frontend/ProductDetails.client.tsxpackages/blocks/product-details/src/frontend/ProductDetails.types.ts
📚 Learning: 2025-11-28T14:07:04.474Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/blocks/product-list/src/frontend/ProductList.client.stories.tsx:15-18
Timestamp: 2025-11-28T14:07:04.474Z
Learning: Mock JWT tokens with placeholder data (e.g., Jane Doe, Acme Corporation) used in Storybook story files (.stories.tsx) for demo/test purposes are acceptable and do not need to be replaced with non-JWT placeholders.
Applied to files:
packages/blocks/recommended-products/src/frontend/RecommendedProducts.client.stories.tsxpackages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx
🧬 Code graph analysis (27)
packages/framework/src/modules/cms/models/blocks/recommended-products.model.ts (1)
packages/blocks/recommended-products/src/api-harmonization/recommended-products.model.ts (1)
RecommendedProductsBlock(25-29)
packages/blocks/recommended-products/src/sdk/index.ts (2)
packages/framework/src/sdk.ts (1)
extendSdk(140-148)packages/blocks/recommended-products/src/sdk/recommended-products.ts (1)
recommendedProducts(8-34)
packages/integrations/mocked/src/modules/cms/mappers/mocks/pages/product-details.page.ts (1)
apps/api-harmonization/src/modules/page/page.model.ts (1)
Page(47-50)
packages/framework/src/modules/cms/cms.model.ts (4)
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
ProductDetailsBlock(34-39)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
ProductDetailsBlock(12-15)packages/blocks/recommended-products/src/api-harmonization/recommended-products.model.ts (1)
RecommendedProductsBlock(25-29)packages/framework/src/modules/cms/models/blocks/recommended-products.model.ts (1)
RecommendedProductsBlock(3-8)
packages/blocks/recommended-products/src/api-harmonization/recommended-products.service.spec.ts (2)
packages/blocks/recommended-products/src/api-harmonization/index.ts (1)
RecommendedProductsService(4-4)packages/framework/src/modules/resources/resources.model.ts (1)
Service(30-34)
packages/blocks/recommended-products/src/api-harmonization/recommended-products.request.ts (1)
packages/framework/src/modules/cms/cms.request.ts (1)
GetCmsEntryParams(5-8)
packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.recommended-products.mapper.ts (4)
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.recommended-products.mapper.ts (1)
mapRecommendedProductsBlock(27-40)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.recommended-products.mapper.ts (1)
mapRecommendedProductsBlock(3-15)packages/blocks/recommended-products/src/api-harmonization/recommended-products.model.ts (1)
RecommendedProductsBlock(25-29)packages/framework/src/modules/cms/models/blocks/recommended-products.model.ts (1)
RecommendedProductsBlock(3-8)
packages/blocks/recommended-products/src/api-harmonization/recommended-products.controller.ts (4)
packages/blocks/recommended-products/src/api-harmonization/index.ts (1)
URL(1-1)packages/blocks/recommended-products/src/api-harmonization/recommended-products.client.ts (1)
URL(1-1)packages/framework/src/utils/models/headers.ts (1)
AppHeaders(1-6)packages/blocks/recommended-products/src/api-harmonization/recommended-products.request.ts (1)
GetRecommendedProductsBlockQuery(3-7)
packages/blocks/recommended-products/src/frontend/RecommendedProducts.types.ts (2)
packages/blocks/recommended-products/src/api-harmonization/recommended-products.model.ts (1)
RecommendedProductsBlock(25-29)packages/framework/src/modules/cms/models/blocks/recommended-products.model.ts (1)
RecommendedProductsBlock(3-8)
packages/blocks/recommended-products/src/frontend/RecommendedProducts.client.stories.tsx (2)
packages/blocks/recommended-products/src/frontend/RecommendedProducts.client.tsx (1)
RecommendedProductsPure(10-27)packages/blocks/recommended-products/src/frontend/index.ts (1)
RecommendedProductsPure(1-1)
packages/framework/src/modules/cms/cms.service.ts (5)
packages/framework/src/modules/cms/cms.request.ts (1)
GetCmsEntryParams(5-8)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
ProductDetailsBlock(34-39)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
ProductDetailsBlock(12-15)packages/blocks/recommended-products/src/api-harmonization/recommended-products.model.ts (1)
RecommendedProductsBlock(25-29)packages/framework/src/modules/cms/models/blocks/recommended-products.model.ts (1)
RecommendedProductsBlock(3-8)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (7)
packages/blocks/product-details/src/frontend/ProductDetails.types.ts (1)
ProductDetailsPureProps(13-13)packages/ui/src/components/ProductGallery/ProductGallery.tsx (1)
ProductGallery(19-252)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
Badge(5-8)packages/blocks/recommended-products/src/api-harmonization/recommended-products.model.ts (1)
Badge(5-8)packages/ui/src/components/DynamicIcon/DynamicIcon.tsx (1)
DynamicIcon(8-42)packages/ui/src/components/RichText/RichText.tsx (1)
RichText(48-252)packages/framework/src/utils/models/price.ts (1)
Price(3-7)
packages/blocks/product-details/src/frontend/ProductDetails.types.ts (3)
apps/docs/src/components/ClientsSection/index.tsx (1)
Client(3-6)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
ProductDetailsBlock(34-39)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
ProductDetailsBlock(12-15)
packages/blocks/recommended-products/src/api-harmonization/recommended-products.module.ts (3)
apps/api-harmonization/src/app.module.ts (1)
Module(85-162)packages/blocks/recommended-products/src/api-harmonization/index.ts (3)
RecommendedProductsBlockModule(3-3)RecommendedProductsService(4-4)RecommendedProductsController(5-5)packages/framework/src/modules/products/products.model.ts (1)
Products(42-42)
packages/blocks/recommended-products/src/frontend/RecommendedProducts.client.tsx (2)
packages/blocks/recommended-products/src/frontend/index.ts (1)
RecommendedProductsPure(1-1)packages/blocks/recommended-products/src/frontend/RecommendedProducts.types.ts (1)
RecommendedProductsPureProps(13-13)
apps/api-harmonization/src/modules/page/page.model.ts (8)
packages/blocks/recommended-products/src/frontend/RecommendedProducts.server.tsx (1)
RecommendedProducts(12-43)packages/blocks/recommended-products/src/frontend/index.ts (1)
RecommendedProducts(2-2)packages/blocks/recommended-products/src/api-harmonization/recommended-products.model.ts (1)
RecommendedProductsBlock(25-29)packages/framework/src/modules/cms/models/blocks/recommended-products.model.ts (1)
RecommendedProductsBlock(3-8)packages/blocks/product-details/src/frontend/ProductDetails.server.tsx (1)
ProductDetails(12-36)packages/blocks/product-details/src/frontend/index.ts (1)
ProductDetails(2-2)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
ProductDetailsBlock(34-39)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
ProductDetailsBlock(12-15)
packages/blocks/recommended-products/src/api-harmonization/recommended-products.mapper.ts (1)
packages/blocks/recommended-products/src/api-harmonization/recommended-products.model.ts (3)
RecommendedProductsBlock(25-29)ProductSummary(10-18)Labels(20-23)
packages/blocks/recommended-products/lint-staged.config.mjs (1)
packages/blocks/recommended-products/.prettierrc.mjs (1)
config(8-23)
packages/blocks/recommended-products/src/api-harmonization/recommended-products.service.ts (6)
packages/framework/src/modules/products/products.model.ts (1)
Products(42-42)packages/blocks/recommended-products/src/api-harmonization/recommended-products.request.ts (1)
GetRecommendedProductsBlockQuery(3-7)packages/framework/src/utils/models/headers.ts (1)
AppHeaders(1-6)packages/blocks/recommended-products/src/api-harmonization/recommended-products.model.ts (3)
RecommendedProductsBlock(25-29)ProductSummary(10-18)Badge(5-8)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (2)
Product(10-15)Badge(5-8)packages/blocks/recommended-products/src/api-harmonization/recommended-products.mapper.ts (1)
mapRecommendedProducts(5-21)
packages/integrations/mocked/src/modules/cms/mappers/cms.page.mapper.ts (1)
packages/integrations/mocked/src/modules/cms/mappers/mocks/pages/product-details.page.ts (3)
PAGE_PRODUCT_DETAILS_EN(3-45)PAGE_PRODUCT_DETAILS_DE(47-89)PAGE_PRODUCT_DETAILS_PL(91-133)
packages/blocks/product-details/src/frontend/ProductDetails.renderer.tsx (3)
packages/blocks/product-details/src/frontend/index.ts (2)
ProductDetailsRenderer(3-3)ProductDetails(2-2)packages/blocks/product-details/src/frontend/ProductDetails.types.ts (1)
ProductDetailsRendererProps(15-17)packages/blocks/product-details/src/frontend/ProductDetails.server.tsx (1)
ProductDetails(12-36)
packages/blocks/product-details/src/frontend/ProductDetails.server.tsx (5)
packages/blocks/product-details/src/frontend/index.ts (1)
ProductDetails(2-2)packages/blocks/product-details/src/frontend/ProductDetails.types.ts (1)
ProductDetailsProps(5-11)packages/blocks/recommended-products/src/sdk/index.ts (1)
sdk(24-28)packages/blocks/product-details/src/sdk/index.ts (1)
sdk(24-28)apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx (2)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (1)
ProductDetailsPure(18-237)packages/blocks/product-details/src/frontend/index.ts (1)
ProductDetailsPure(1-1)
packages/blocks/recommended-products/src/frontend/RecommendedProducts.server.tsx (3)
packages/blocks/recommended-products/src/frontend/index.ts (1)
RecommendedProducts(2-2)packages/blocks/recommended-products/src/frontend/RecommendedProducts.types.ts (1)
RecommendedProductsProps(5-11)packages/blocks/recommended-products/src/sdk/index.ts (1)
sdk(24-28)
apps/frontend/src/blocks/renderBlocks.tsx (4)
packages/blocks/product-details/src/frontend/ProductDetails.server.tsx (1)
ProductDetails(12-36)packages/blocks/product-details/src/frontend/index.ts (1)
ProductDetails(2-2)packages/blocks/recommended-products/src/frontend/RecommendedProducts.server.tsx (1)
RecommendedProducts(12-43)packages/blocks/recommended-products/src/frontend/index.ts (1)
RecommendedProducts(2-2)
packages/blocks/recommended-products/vitest.config.mjs (1)
packages/blocks/recommended-products/.prettierrc.mjs (1)
config(8-23)
packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (4)
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(39-50)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(3-17)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
ProductDetailsBlock(34-39)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
ProductDetailsBlock(12-15)
🪛 GitHub Check: CodeQL
packages/integrations/mocked/src/modules/cms/mappers/cms.page.mapper.ts
[failure] 161-161: Polynomial regular expression used on uncontrolled data
This regular expression that depends on library input may run slow on strings with many repetitions of 'a'.
[failure] 167-167: Polynomial regular expression used on uncontrolled data
This regular expression that depends on library input may run slow on strings with many repetitions of 'a'.
[failure] 173-173: Polynomial regular expression used on uncontrolled data
This regular expression that depends on library input may run slow on strings with many repetitions of 'a'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (42)
packages/blocks/recommended-products/.gitignore (1)
1-57: Standard and comprehensive .gitignore configuration. ✓The gitignore follows Node.js project best practices with appropriate coverage of compiled outputs (dist, node_modules, build), logging, OS artifacts, test coverage, IDE/editor configs, environment files, temp directories, and runtime data. The patterns align well with the monorepo structure and multiple build outputs (api-harmonization, frontend, sdk) mentioned in the PR context. The VSCode exception handling (lines 33-36) is properly configured.
packages/blocks/recommended-products/lint-staged.config.mjs (1)
1-3: LGTM! Clean re-export of shared lint-staged configuration.This follows the monorepo pattern of centralizing lint-staged configuration through the shared base config, ensuring consistency across block packages.
packages/blocks/recommended-products/.prettierrc.mjs (1)
1-25: LGTM! Clean Prettier configuration following established patterns.The configuration correctly maps the three source directories (api-harmonization, frontend, sdk) to their appropriate Prettier presets. The structure is consistent with the product-details block and properly reuses shared configuration from
@o2s/prettier-config.packages/blocks/recommended-products/vitest.config.mjs (1)
1-1: LGTM! Configuration re-export follows the established pattern.The code correctly imports and re-exports the shared Vitest configuration from
@o2s/vitest-config/block, maintaining consistency with the product-details block and ensuring uniform test setup across block packages.packages/blocks/recommended-products/eslint.config.mjs (1)
1-18: LGTM! Correctly structured ESLint flat config following repository patterns.The configuration properly uses ESLint v9+
defineConfigwithextendsarrays and appropriately segments linting rules by code surface (API harmonization, frontend, SDK). The required@o2s/eslint-configdependency is properly declared in devDependencies.packages/blocks/product-details/src/frontend/ProductDetails.types.ts (1)
1-17: LGTM! Type definitions are well-structured.The type definitions properly establish the props hierarchy for the ProductDetails block. The use of
ReturnType<typeof defineRouting>ensures type safety for the routing config, and the composition pattern (intersection and Omit) appropriately differentiates between renderer, server, and client prop requirements.packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx (1)
1-144: LGTM! Comprehensive story coverage.The Storybook stories provide excellent coverage of the ProductDetails component's key variants. The mock data is realistic and well-structured, demonstrating the component's flexibility with different button configurations.
packages/blocks/product-details/src/frontend/ProductDetails.server.tsx (1)
12-36: LGTM! Server component pattern is correct.The async server component properly fetches data via SDK and dynamically renders the client component. The error handling logs errors and gracefully degrades by returning null.
Consider whether returning
nullon error provides sufficient feedback to users. Depending on your product requirements, you might want to render an error boundary or fallback UI instead of silently failing.packages/blocks/product-details/src/frontend/ProductDetails.renderer.tsx (1)
10-39: LGTM! Renderer implementation is correct.The renderer properly bridges the server component with Suspense boundaries and provides appropriate loading fallbacks. The locale resolution pattern (preferring prop over hook) is flexible and correct. The guard for
slug[1]prevents runtime errors when the product ID segment is missing.packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (1)
18-237: LGTM! Client component implementation is well-structured.The responsive two-column layout is well-crafted with appropriate mobile/desktop breakpoints. The component properly handles conditional rendering for optional fields (badges, description, specs, location) and provides good UX with sticky sidebar and mobile bottom CTA.
packages/integrations/mocked/src/modules/resources/mock/products.mock.ts (1)
35-35: The migration from absolute URLs to relative paths (/products/{id}) is properly supported by the application.All product links have been consistently updated to use the
/products/{id}pattern. The CMS mapper explicitly handles these routes with pattern matching (/\/products\/.+/) and returns the product-details page template configured with both the ProductDetailsBlock and RecommendedProductsBlock. The application's catch-all route dynamically resolves these paths via the CMS API, confirming the routing infrastructure is in place for all 11 mock product updates (lines 35, 70, 115, 156, 182, 210, 238, 269, 301, 329, 356).packages/integrations/mocked/src/modules/cms/mappers/cms.page.mapper.ts (4)
39-43: LGTM!Import statements correctly follow the existing pattern for locale-specific page imports.
303-303: LGTM!Product details page correctly added to the Polish locale pages array.
326-326: LGTM!Product details page correctly added to the German locale pages array.
349-349: LGTM!Product details page correctly added to the English locale pages array.
packages/integrations/mocked/src/modules/cms/mappers/mocks/pages/product-details.page.ts (3)
1-45: LGTM!The English product details page configuration is well-structured with appropriate SEO metadata, permissions, and template blocks.
47-89: LGTM!The German product details page configuration has correct localized slugs and SEO metadata.
91-133: LGTM!The Polish product details page configuration has correct localized slugs and SEO metadata.
packages/blocks/recommended-products/tsconfig.json (1)
1-18: LGTM! Configuration follows the composite build pattern.The project references correctly coordinate the frontend, API, and SDK builds. The configuration appropriately extends the base config and enables composite mode for project references.
packages/blocks/recommended-products/tsconfig.frontend.json (1)
15-21: LGTM! Include paths support frontend-to-API type sharing.The frontend configuration appropriately includes API harmonization files and SDK sources, enabling type-safe imports of shared models and clients in frontend components.
packages/blocks/recommended-products/tsconfig.api.json (1)
1-14: LGTM! API harmonization config is properly scoped.The configuration correctly isolates the API harmonization layer with appropriate output directory and base URL settings. Compiler options are suitable for the API build target.
packages/blocks/recommended-products/tsconfig.sdk.json (1)
13-18: No action required — therecommended-products.url.tsfile does not exist in the codebase.The file mentioned in the AI summary (
src/api-harmonization/recommended-products.url.ts) does not exist. The api-harmonization directory contains only:index.ts,recommended-products.client.ts,recommended-products.controller.ts,recommended-products.mapper.ts,recommended-products.model.ts,recommended-products.module.ts,recommended-products.request.ts,recommended-products.service.spec.ts, andrecommended-products.service.ts. The current tsconfig.sdk.json include array is accurate.Likely an incorrect or invalid review comment.
apps/frontend/src/blocks/renderBlocks.tsx (2)
20-23: LGTM! Clean block integration.The imports follow the established pattern for block modules and align with the frontend package structure.
147-150: LGTM! Switch cases correctly integrated.The new block cases follow the same pattern as existing blocks and correctly reference the Renderer components from the imported modules.
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.recommended-products.mapper.ts (1)
27-40: LGTM! Locale switching implemented correctly.The function follows the established pattern for locale-based mock data with appropriate fallback handling.
packages/blocks/recommended-products/src/sdk/index.ts (2)
24-28: LGTM! SDK extension follows established pattern.The extension correctly wires the
getRecommendedProductsmethod following the same pattern used for other block SDKs.
15-22: This is a framework-level type definition issue affecting multiple SDK files.The
@ts-expect-errorsuppressions forlogger.levelandlogger.formatare necessary due to incomplete type definitions in the@o2s/framework/sdkpackage. This same pattern appears consistently across 20+ SDK files (apps/frontend/src/api/sdk.ts, all packages/blocks/*/src/sdk/index.ts, packages/modules/surveyjs/src/sdk/index.ts, etc.), indicating the properties are not properly typed in the framework package.The suppressions are appropriate and justified. However, resolving this requires updating the framework package's type definitions rather than addressing individual SDK files. Consider filing an issue with the framework maintainers to include proper types for
logger.levelandlogger.format.apps/api-harmonization/src/modules/page/page.model.ts (2)
19-22: LGTM! Imports follow established pattern.The imports correctly reference the api-harmonization modules for both new blocks.
83-84: LGTM! Union members correctly use __typename.The new block types are properly added to the discriminated union using the __typename field from the API harmonization layer, consistent with the learned pattern that __typename discriminators belong at this layer, not the framework layer.
Based on learnings, the __typename field is correctly used at the API harmonization layer for discriminated unions.
packages/blocks/recommended-products/src/frontend/RecommendedProducts.types.ts (1)
1-18: LGTM! Type definitions are well-structured.The type hierarchy appropriately separates concerns:
RecommendedProductsPropsfor base component propsRecommendedProductsPurePropsfor client-side pure component combining props with block dataRecommendedProductsRendererPropsfor renderer, transforminglocalefrom required to optionalThe pattern of omitting
localeand re-adding it as optional (lines 15-17) is intentional for the renderer context.packages/blocks/recommended-products/src/sdk/recommended-products.ts (1)
8-34: LGTM! SDK implementation follows established patterns.The SDK method correctly:
- Merges query parameters with the required
id- Handles optional headers and authorization
- Returns properly typed Promise
packages/framework/src/modules/cms/cms.model.ts (1)
47-48: LGTM!The new exports for
ProductDetailsBlockandRecommendedProductsBlockfollow the established pattern and integrate correctly with the existing block export structure.packages/framework/src/modules/cms/cms.service.ts (1)
132-142: LGTM!The three new abstract methods follow the established pattern for block retrieval methods. The signatures are consistent with existing block getters, using the correct parameter and return types.
packages/blocks/recommended-products/src/api-harmonization/recommended-products.module.ts (1)
9-29: LGTM! Module structure follows established patterns.The module registration pattern, provider aliases, and exports are consistent with the product-details block and framework conventions. The unused
_configparameter maintains API consistency for future extensibility.packages/blocks/recommended-products/src/api-harmonization/recommended-products.mapper.ts (1)
5-21: LGTM! Mapper correctly implements API harmonization pattern.The mapper correctly adds the
__typenamediscriminator at the API harmonization layer (as per learnings) and uses optional chaining for label extraction. The unused_localeparameter maintains API consistency for future localization needs.packages/blocks/recommended-products/src/api-harmonization/index.ts (1)
1-8: LGTM! Clean public API surface.The façade pattern with aliased exports and namespace consolidation follows established conventions and provides a clear, maintainable public API.
packages/blocks/recommended-products/src/frontend/RecommendedProducts.client.tsx (1)
14-16: ProductCarousel already handles optional labels gracefully—no action needed.ProductCarousel's prop types explicitly define
titleandlinkDetailsLabelas optional properties (title?: string,linkDetailsLabel?: string). The component uses conditional rendering ({(title || description || action) && ...}) to handle undefined values, so passinglabels.titleandlabels.detailsLabelis safe even when they are undefined.Likely an incorrect or invalid review comment.
packages/blocks/recommended-products/src/frontend/RecommendedProducts.renderer.tsx (1)
36-42: No action needed. The server component already gracefully handles undefinedexcludeProductIdthrough its optional prop typing. TheexcludeProductIdprop is explicitly declared as optional (excludeProductId?: string) in theRecommendedProductsPropsinterface, the API request query parameters, and the backend query definition. Undefined values flow through the component chain safely by design.packages/blocks/recommended-products/src/api-harmonization/recommended-products.controller.ts (1)
17-24: Public access pattern is intentional for block endpoints.The empty
roles: []is the framework's designed way to allow public access. TheRolesGuardexplicitly returnstruewhen the roles array is empty, and all block endpoints (recommended-products, product-details, ticket-summary, hero-section, etc.) consistently use this pattern. This is correct.packages/blocks/recommended-products/src/api-harmonization/recommended-products.model.ts (2)
5-23: LGTM! Well-structured type definitions.The type definitions follow good TypeScript practices:
- String literal unions for the
variantfield ensure type safety- Proper reuse of existing
Products.Model.Producttypes forimageandprice- Optional fields are clearly marked
- Type structure is clear and maintainable
25-29: LGTM! Correct placement of__typenamediscriminator.The
RecommendedProductsBlocktype correctly includes the__typenamediscriminator in the API harmonization layer. This aligns with the established architecture pattern where__typenamefields are added at the API harmonization layer for discriminated unions, not in the framework layer.Based on learnings, this is the appropriate location for this discriminator.
packages/blocks/recommended-products/src/frontend/RecommendedProducts.server.tsx (1)
29-38: No action needed — prop spreading pattern is correct.The spread operator followed by explicit props is intentional and correct. The SDK response (from
data) providesproductsandlabelsfrom theModel.RecommendedProductsBlock, while the explicit props (id,excludeProductId,limit,locale,routing) enrich the response with request context. The component destructures these as{ locale, routing, ...component }, wherecomponentreceives the remaining props including the API response data. No duplication or conflict occurs.
packages/blocks/recommended-products/src/api-harmonization/recommended-products.service.ts
Outdated
Show resolved
Hide resolved
packages/blocks/recommended-products/src/api-harmonization/recommended-products.service.ts
Show resolved
Hide resolved
packages/blocks/recommended-products/src/frontend/RecommendedProducts.renderer.tsx
Outdated
Show resolved
Hide resolved
packages/framework/src/modules/cms/models/blocks/recommended-products.model.ts
Show resolved
Hide resolved
...ges/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/blocks/product-details/src/frontend/ProductDetails.client.tsx:
- Around line 18-27: ProductDetailsPure currently destructures product from
component and immediately accesses product.keySpecs, which will throw if product
is null/undefined; update the component to defensively handle a missing product
by either returning early (e.g., if (!product) return null or a placeholder) or
by using optional chaining/defaults (e.g., const keySpecs = product?.keySpecs ??
[]), and ensure any other accesses to product within ProductDetailsPure are
similarly guarded or use a safe fallback.
- Around line 202-203: In ProductDetails.client.tsx update the bottom fixed
div's utility classes: replace the invalid Tailwind class "z-100" with a valid
scale value like "z-50", and change the inner container's "max-w-7xl ml-auto
mr-4" to symmetric spacing such as "max-w-7xl mx-auto px-4" to ensure consistent
centering and horizontal padding.
🧹 Nitpick comments (4)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (4)
47-55: Consider using stable keys for badge rendering.The badges are mapped using
indexas the key. While this works if the badge array is static, consider using a more stable identifier (e.g.,badge.idor a combination ofbadge.labelandbadge.variant) if badges can be reordered or modified dynamically.♻️ Alternative approach with stable keys
If badges have unique labels:
- {product.badges.map((badge, index) => ( - <li key={index}> + {product.badges.map((badge) => ( + <li key={`${badge.variant}-${badge.label}`}> <Badge variant={badge.variant}>{badge.label}</Badge> </li> ))}
60-79: Consider using stable keys for spec items.Similar to the badges, key specs are mapped using
indexas the key. If specs can be dynamically modified, consider using a more stable identifier.
162-195: Consider extracting action button rendering logic.The action button rendering logic is duplicated between the desktop sidebar (lines 162-195) and mobile bottom bar (lines 200-234). While the mobile version intentionally omits the icon, the core href/onClick logic is repeated.
♻️ Proposed refactor: Extract common logic
Consider creating a helper function or component:
const renderActionButton = (options: { actionButton: typeof actionButton; LinkComponent: typeof LinkComponent; variant?: string; size: 'lg' | 'default'; showIcon?: boolean; className?: string; }) => { const { actionButton, LinkComponent, variant, size, showIcon = true, className } = options; if (!actionButton) return null; const buttonContent = ( <> {showIcon && actionButton.icon && ( <DynamicIcon name={actionButton.icon} size={20} className="mr-2" /> )} {actionButton.label} </> ); const buttonProps = { variant: actionButton.variant || variant || 'default', size, className, }; if (actionButton.href) { return ( <Button {...buttonProps} asChild> <LinkComponent href={actionButton.href}>{buttonContent}</LinkComponent> </Button> ); } return ( <Button {...buttonProps} onClick={actionButton.onClick}> {buttonContent} </Button> ); };Then use it in both places with different options.
Also applies to: 200-234
232-232: Consider matching spacer height to actual bottom bar height.The fixed spacer height of
h-32(8rem/128px) may not match the actual height of the mobile bottom bar, which varies based on its content (price display + button + padding). This could result in either excessive whitespace or content being hidden.💡 Alternative approaches
Option 1: Calculate the actual height and use a CSS variable:
// Use a ref to measure the bar height dynamically const bottomBarRef = useRef<HTMLDivElement>(null); const [barHeight, setBarHeight] = useState(0); useEffect(() => { if (bottomBarRef.current) { setBarHeight(bottomBarRef.current.offsetHeight); } }, [actionButton]);Option 2: Use
pb-32(padding-bottom) on the parent container instead of a separate spacer div, which is more semantic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: deploy-preview
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (3)
1-16: LGTM!The imports are well-organized and appropriate for a client-side React component. The 'use client' directive is correctly placed at the top of the file.
81-109: LGTM!The description and detailed specifications sections are well-structured with proper conditional rendering and semantic HTML usage. The
asChildpattern ensures proper heading hierarchy for accessibility.
18-237: Overall component structure is solid.The component demonstrates good practices:
- Semantic HTML with proper list elements (
ul/li) for accessibility- Responsive design with appropriate mobile and desktop layouts
- Proper use of
hasPriorityprop for LCP optimization via gallery preloading- Type-safe with TypeScript
The main areas for improvement have been flagged in separate comments above.
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/blocks/product-details/src/frontend/ProductDetails.client.tsx:
- Line 188: The Tailwind class "z-100" on the mobile footer div generates no CSS
because the project doesn't define that utility; replace it with the arbitrary
value syntax "z-[100]" in the same class string on the div (the element
rendering in ProductDetails.client.tsx that has className "lg:hidden fixed
bottom-0 left-0 right-0 bg-background border-t border-border p-4 shadow-lg
z-100") so the element receives the intended z-index.
🧹 Nitpick comments (8)
packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (3)
47-47: Consider reducing the bottom margin.The
mb-12(3rem/48px) creates significant space below each product card, likely to prevent pagination bullets from overlapping. Consider whether this spacing could be reduced or managed through padding on the carousel container for a more compact layout.Alternative approach
If the margin is solely for pagination clearance, you might refactor to use carousel container padding instead:
- <div key={product.id} className="h-full mb-12"> + <div key={product.id} className="h-full">And adjust the carousel className:
className={cn( - '[&_.swiper-slide]:h-auto [&_.swiper-pagination]:bottom-0 [&_.swiper-pagination-bullet]:bg-primary [&_.swiper-pagination-bullet-active]:bg-primary [&_.swiper-pagination-bullet]:opacity-100 [&_.swiper-pagination-bullet]:w-2.5 [&_.swiper-pagination-bullet]:h-2.5', + '[&_.swiper-slide]:h-auto [&_.swiper]:pb-12 [&_.swiper-pagination]:bottom-0 [&_.swiper-pagination-bullet]:bg-primary [&_.swiper-pagination-bullet-active]:bg-primary [&_.swiper-pagination-bullet]:opacity-100 [&_.swiper-pagination-bullet]:w-2.5 [&_.swiper-pagination-bullet]:h-2.5', carouselClassName, )}However, verify this spacing is appropriate for your design system before changing.
70-73: Consider extracting the complex className for maintainability.The className string on line 71 contains multiple Swiper-specific selectors that make it difficult to read and modify. Consider extracting this to a constant or breaking it into a more maintainable structure.
Refactoring suggestion
+const SWIPER_PAGINATION_STYLES = [ + '[&_.swiper-slide]:h-auto', + '[&_.swiper-pagination]:bottom-0', + '[&_.swiper-pagination-bullet]:bg-primary', + '[&_.swiper-pagination-bullet-active]:bg-primary', + '[&_.swiper-pagination-bullet]:opacity-100', + '[&_.swiper-pagination-bullet]:w-2.5', + '[&_.swiper-pagination-bullet]:h-2.5', +].join(' '); + export const ProductCarousel: React.FC<ProductCarouselProps> = ({ // ... props }) => { // ... earlier code <Carousel // ... other props className={cn( - '[&_.swiper-slide]:h-auto [&_.swiper-pagination]:bottom-0 [&_.swiper-pagination-bullet]:bg-primary [&_.swiper-pagination-bullet-active]:bg-primary [&_.swiper-pagination-bullet]:opacity-100 [&_.swiper-pagination-bullet]:w-2.5 [&_.swiper-pagination-bullet]:h-2.5', + SWIPER_PAGINATION_STYLES, carouselClassName, )}
66-87: Note: Redundant configuration between base props and breakpoints.Lines 66-67 set
slidesPerView={1}andspaceBetween={16}, which are then overridden by the0breakpoint in lines 75-77. While this works correctly (breakpoints take precedence), it creates redundancy. Consider either removing the base values or documenting that breakpoints override them.Simplification option
Remove the redundant base values since breakpoints cover all screen sizes:
<Carousel slides={...} - slidesPerView={1} - spaceBetween={16} showNavigation={true} showPagination={true}The
breakpointsobject starting at0will handle the default case.packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (1)
24-24:createNavigationis called on every render, which may impact performance.
createNavigation(routing)creates a new navigation context on each render. Consider memoizing this or moving it outside the component ifroutingis stable.♻️ Suggested refactor
export const ProductDetailsPure: React.FC<ProductDetailsPureProps> = ({ locale, routing, hasPriority, ...component }) => { - const { Link: LinkComponent } = createNavigation(routing); + const { Link: LinkComponent } = React.useMemo(() => createNavigation(routing), [routing]); const { product, labels, actionButton } = component;packages/blocks/product-details/src/api-harmonization/product-details.model.ts (2)
10-15: Compleximagestype may not reflect runtime behavior accurately.The conditional type logic on lines 12-14 is hard to reason about. The mapper handles this at runtime with
product.images || (product.image ? [product.image] : []), which can result in an empty array[]— but the current type definition doesn't account for that.Consider simplifying to match actual runtime possibilities:
♻️ Suggested simplification
export type Product = Products.Model.Product & { badges?: Badge[]; - images: - | NonNullable<Products.Model.Product['images']> - | (Products.Model.Product['image'] extends undefined ? never : [Products.Model.Product['image']]); + images: NonNullable<Products.Model.Product['images']> | Products.Model.Product['image'][]; };
5-22: Consider extracting the sharedvariantunion type.
Badge.variantandActionButton.variantuse the same union. Extracting to a shared type would improve maintainability.♻️ Suggested refactor
+export type ButtonVariant = 'default' | 'secondary' | 'destructive' | 'outline'; + export type Badge = { label: string; - variant: 'default' | 'secondary' | 'destructive' | 'outline'; + variant: ButtonVariant; }; // ... export type ActionButton = { label: string; href: string; - variant?: 'default' | 'secondary' | 'destructive' | 'outline'; + variant?: ButtonVariant; icon?: string; };packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (2)
13-17: Unsafe type cast fortag.variantcould cause runtime issues.The cast
as Model.Badge['variant']assumestag.variantis always one of the expected values. If the source data contains an unexpected variant, this could lead to incorrect UI rendering.♻️ Suggested fix with validation
+const VALID_BADGE_VARIANTS = ['default', 'secondary', 'destructive', 'outline'] as const; + +const isValidBadgeVariant = (variant: unknown): variant is Model.Badge['variant'] => + VALID_BADGE_VARIANTS.includes(variant as Model.Badge['variant']); + export const mapProductDetails = ( product: Products.Model.Product, cms: CMS.Model.ProductDetailsBlock.ProductDetailsBlock, ): Model.ProductDetailsBlock => { const mappedProduct: Model.Product = { ...product, images: product.images || (product.image ? [product.image] : []), badges: product.tags?.map((tag) => ({ label: tag.label, - variant: tag.variant as Model.Badge['variant'], + variant: isValidBadgeVariant(tag.variant) ? tag.variant : 'default', })) || [],
35-43: Hardcoded icon'MessageCircle'may limit flexibility.The action button icon is hardcoded. If different product types need different icons, consider making this configurable via CMS labels or product data.
♻️ Optional: Make icon configurable
actionButton: labels.actionButtonLabel && product.link ? { label: labels.actionButtonLabel, href: product.link, variant: 'default', - icon: 'MessageCircle', + icon: cms.labels.actionButtonIcon || 'MessageCircle', } : undefined,This would require adding
actionButtonIcon?: stringto the CMS Labels type.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/blocks/product-details/src/api-harmonization/product-details.mapper.tspackages/blocks/product-details/src/api-harmonization/product-details.model.tspackages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsxpackages/blocks/product-details/src/frontend/ProductDetails.client.tsxpackages/ui/src/components/ProductCarousel/ProductCarousel.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
📚 Learning: 2025-11-26T11:57:00.632Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
Applied to files:
packages/blocks/product-details/src/api-harmonization/product-details.mapper.tspackages/blocks/product-details/src/api-harmonization/product-details.model.ts
🧬 Code graph analysis (2)
packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (4)
packages/ui/src/components/ProductCarousel/ProductCarousel.types.ts (1)
ProductCarouselProps(8-17)packages/ui/src/components/RichText/RichText.tsx (1)
RichText(48-252)packages/ui/src/components/Cards/ProductCard/ProductCard.tsx (1)
ProductCard(16-94)packages/ui/src/lib/utils.ts (1)
cn(5-7)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (5)
packages/blocks/product-details/src/frontend/ProductDetails.types.ts (1)
ProductDetailsPureProps(13-13)packages/ui/src/components/ProductGallery/ProductGallery.tsx (1)
ProductGallery(19-252)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
Badge(5-8)packages/ui/src/components/DynamicIcon/DynamicIcon.tsx (1)
DynamicIcon(8-42)packages/ui/src/components/RichText/RichText.tsx (1)
RichText(48-252)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (7)
packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (4)
1-24: LGTM! Clean component structure and imports.The 'use client' directive is correctly placed for React 19 Server Components architecture, and all imports are properly organized.
25-27: LGTM! Appropriate guard clause.The early return correctly prevents rendering an empty carousel when no products are available.
32-42: LGTM! Well-structured header section.The nested conditionals ensure the header wrapper only renders when needed, and the Typography variant="h2" provides proper semantic structure.
54-61: Verify link rendering behavior.The link object is only created when
linkDetailsLabelis provided. This means a product with aproduct.linkbut nolinkDetailsLabelwon't render a clickable link. Confirm this is the intended UX—some users might expect the product card itself or the product name to be clickable even without an explicit detail link.packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (1)
29-211: Well-structured responsive layout with proper semantic HTML.The component demonstrates good practices:
- Proper heading hierarchy (h1, h2) with
asChildpattern- Semantic list elements for badges and specs
- Mobile-first responsive design with appropriate breakpoints
- Sticky sidebar for desktop and fixed bottom CTA for mobile
- Conditional rendering for optional fields
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
33-38: Correct placement of__typenamediscriminator.Per project conventions, the
__typenamefield is correctly added at the API harmonization layer for discriminated unions, rather than in the framework layer. Based on learnings, this is the expected pattern.packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (1)
5-46: Mapper logic is clean with appropriate fallbacks.The mapping function handles optional fields well with sensible defaults (
|| []for arrays). The conditionalactionButtoncreation based on bothactionButtonLabelandproduct.linkprevents incomplete button states.
What does this PR do?
Related Ticket(s)
Key Changes
How to test
Media (Loom or gif)
Summary by CodeRabbit
New Features
UI Components
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.